Skip to content

Remove functions marked as undocumented and unused#10096

Closed
BenBrostoff wants to merge 1 commit into
react:masterfrom
BenBrostoff:remove-unused-test-functions
Closed

Remove functions marked as undocumented and unused#10096
BenBrostoff wants to merge 1 commit into
react:masterfrom
BenBrostoff:remove-unused-test-functions

Conversation

@BenBrostoff

@BenBrostoff BenBrostoff commented Jul 3, 2017

Copy link
Copy Markdown

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.

aweary
aweary suggested changes Jul 3, 2017
View reviewed changes

@aweary aweary left a comment

Copy link
Copy Markdown
Contributor

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

gaearon commented Jul 6, 2017 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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

BenBrostoff commented Jul 6, 2017

Copy link
Copy Markdown
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

aweary commented Jul 6, 2017

Copy link
Copy Markdown
Contributor

@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

gaearon commented Jul 6, 2017

Copy link
Copy Markdown
Collaborator

Mind sending a PR to Enzyme to stop importing it?

@aweary aweary mentioned this pull request Jul 6, 2017
Closed
@aweary

aweary commented Jul 6, 2017

Copy link
Copy Markdown
Contributor

@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

aweary commented Jul 7, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@gaearon

gaearon commented Oct 4, 2017

Copy link
Copy Markdown
Collaborator

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

Reviewers

1 more reviewer

@aweary aweary aweary requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@BenBrostoff @gaearon @aweary @facebook-github-bot