Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused isCompositeComponentElement re-export #1023

Closed

Conversation

aweary
Copy link
Collaborator

@aweary aweary commented Jul 6, 2017

This method is being deprecated soon facebook/react#10096

We don't actually use it at all internally, so this change just removes the unused import/export.

The rewrite doesn't include this import/export so this may be a moot PR, but I'm not sure if we intend to make any more minor releases before it's accepted.

@ljharb
Copy link
Member

ljharb commented Jul 7, 2017

Removing an export from any file is semver-major. Instead, let's include this change in the rewrite branch, so that the rewrite can be v3.

@ljharb ljharb added the semver: major Breaking changes label Jul 7, 2017
@aweary
Copy link
Collaborator Author

aweary commented Jul 7, 2017

Semver only dictates major releases for breaking changes to the public API, which should be well defined.

For this system to work, you first need to declare a public API. This may consist of documentation or be enforced by the code itself. Regardless, it is important that this API be clear and precise.

Enzyme has a pretty clear public API defined by the documentation, so I would argue this isn't a breaking change since react-compat exports are not defined as part of the public API (and never should be).

@aweary
Copy link
Collaborator Author

aweary commented Jul 7, 2017

Either way, we can close this if we want to consider it semver major since the rewrite branch doesn't contain this import/export anymore.

@ljharb
Copy link
Member

ljharb commented Jul 7, 2017

Our public API is defined as "everything that can possibly be reachable or observed", regardless of what's in documentation, which is also the way I think all projects should define it.

@aweary
Copy link
Collaborator Author

aweary commented Jul 7, 2017

I think that's an ill-defined way to describe a public API, but if that's the standard for Enzyme so be it. In that case, this PR is unnecessary since the same change will be made in #1007

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: major Breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants