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 functions marked as undocumented and unused #10096

Closed

Conversation

BenBrostoff
Copy link

Hey @gaearon - saw you added these TODOs in this commit about 8 months ago. Figured I'd open a PR if this can now be completed.

Copy link
Contributor

@aweary aweary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @BenBrostoff! While these APIs are undocumented they were still technically accessible as part of the public API. Since there's still a chance these are being used we might want to deprecate them before fully removing them.

cc @gaearon what do you think?

@gaearon
Copy link
Collaborator

gaearon commented Jul 6, 2017

I wouldn't mind deprecating but we need to make sure they're not used by Enzyme.
There should also be a clear migration path. (What should people who use them do?)

@BenBrostoff BenBrostoff force-pushed the remove-unused-test-functions branch from 8088c3f to 82c64a4 Compare July 6, 2017 12:36
@BenBrostoff
Copy link
Author

@gaearon / @aweary Took my best shot at deprecating these. @gaearon - re: clear migration path, my deprecation warning obviously does not describe what this path should be, so this PR is definitely incomplete at this point.

@aweary
Copy link
Contributor

aweary commented Jul 6, 2017

@gaearon we only import and re-export isCompositeComponentElement so it should be safe. We can just remove that unused import. The rewrite also doesn't use these APIs.

@gaearon
Copy link
Collaborator

gaearon commented Jul 6, 2017

Mind sending a PR to Enzyme to stop importing it?

@aweary
Copy link
Contributor

aweary commented Jul 6, 2017

@gaearon opened at enzymejs/enzyme#1023, though the rewrite doesn't contain this import/export so it might just be removed when that's merged instead.

@aweary
Copy link
Contributor

aweary commented Jul 7, 2017

@gaearon the export will be removed in the next major release when enzymejs/enzyme#1007 is merged and released.

@gaearon
Copy link
Collaborator

gaearon commented Oct 4, 2017

I deleted them in the end.

@gaearon gaearon closed this Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants