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

Improve interoperability with other plugins #18

Merged
merged 1 commit into from
Sep 28, 2018

Conversation

alecmev
Copy link
Collaborator

@alecmev alecmev commented Apr 28, 2018

Prompted by airbnb/babel-plugin-inline-react-svg#31.

This should handle all possible scenarios now, except for the edge case outlined in Program.exit and for plugins which use path.node.body.unshift instead of path.unshiftContainer. The difference is that unshiftContainer queues the added node for processing by other plugins, as described here, while when something is added via path.node.body mutation, other plugins will never learn about it, if they don't re-traverse the whole file again.

path.scope.hasBinding doesn't cut it, unfortunately, since Babel doesn't keep it up-to-date during traversal, apparently for performance reasons. A bit more info in this PR: babel/babel#6879

There may be side-effects from the addition of a potentially duplicate import, but none come to my mind at the moment. Maybe this should be a new major version, as to alert people about potential breakage.

@vslinko
Copy link
Owner

vslinko commented May 1, 2018

Hello and thank you for your contribution.

Sorry, I have no time to maintain my github repos, but I have an offer for you.
I can make you a maintainer of this repo and you will merge your PR by yourself.

Waiting for your answer.

@alecmev
Copy link
Collaborator Author

alecmev commented May 9, 2018

Hello @vslinko. Took some time to think about this. I'm okay with handling the merge and the publish myself (would also need to be added as a collaborator on npmjs.com), and will follow through with the teething problems of v2, but I don't wish to completely take over the maintainership of this package.

@vslinko
Copy link
Owner

vslinko commented May 20, 2018

Ok. I'll give you permissions.

@alecmev
Copy link
Collaborator Author

alecmev commented Sep 1, 2018

This has been sitting on my todo list for a while now, and I keep putting it off because it may or may not break somebody's build, and I may or may not have to sink a few evenings into fixing it. So I've decided to leave this PR hanging for now, until people actually start asking for it to get merged.

Note to future self (or whoever): test with Next.js's test suite before merging, since they appear to be the largest non-end-user consumer of this plugin.

@timneutkens
Copy link
Collaborator

@vslinko @jeremejevs I'm the lead maintainer of Next.js and got a few reports of this specific issue. I'd be happy to release this new version for you both if you add me as a maintainer. timneutkens both on GitHub and NPM.

@vslinko
Copy link
Owner

vslinko commented Sep 28, 2018

@timneutkens You have been invited. Thank you for your eagerness to help.

@timneutkens timneutkens merged commit 64fde2e into vslinko:master Sep 28, 2018
@timneutkens
Copy link
Collaborator

timneutkens commented Sep 28, 2018

Awesome, thanks @vslinko, I'm going to release this as a patch 👍 Since all tests pass

@timneutkens
Copy link
Collaborator

@vslinko for some reason I see https://www.npmjs.com/package/babel-plugin-react-require listed under my packages, but the package itself doesn't seem to be aware than I'm one of the people that has access, disallowing me to publish.

@vslinko
Copy link
Owner

vslinko commented Oct 1, 2018

@timneutkens I don't know what else I can do. npmjs.com says that you have an access to write.

2018-10-01 8 pm-15-14

Could you write to npmjs.com support?

@timneutkens
Copy link
Collaborator

Thanks for the reply, prompted me to retry, you did nothing wrong, npm just takes a little longer than I expected to give access rights. I just published:

+ babel-plugin-react-require@3.0.1

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

Successfully merging this pull request may close these issues.

None yet

3 participants