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

add .babelrc to .npmignore #270

Merged
merged 1 commit into from
Nov 18, 2018
Merged

add .babelrc to .npmignore #270

merged 1 commit into from
Nov 18, 2018

Conversation

orzechowskid
Copy link
Contributor

babel applies whatever config is closest to any given source file, meaning a .babelrc provided by this NPM package will overrule a .babelrc belonging to consumers of this package (who may desire or require different presets and plugins for their app).

module consumers should be able to transpile this code in whatever way they see fit
@MatthewHerbst
Copy link
Contributor

Hi @orzechowskid thanks for the PR. The lookup behavior for the .babelrc file searches up to the package root of the current package. react-waypoint will never be in the package root, but in ./node_modules relative to the root. As such, the .babelrc in this repo will never override consumer .babelrcs. Further, we need the .babelrc in this repo to ensure that we can build react-waypoint properly for end-consumers.

Learn more here:
https://babeljs.io/docs/en/babelrc.html#lookup-behavior

@orzechowskid
Copy link
Contributor Author

orzechowskid commented Aug 3, 2018

The PR isn't to remove babel config from the Git repo; it's to remove babel config from the package archive pushed to NPM. I understand that .babelrc is required for development purposes - that's why a change was made to .npmignore instead of .gitignore .

When consumers attempt to transpile an app with react-waypoint as an ES2015 module dependency, the babel config at myproject/node_modules/react-waypoint/.babelrc will be applied instead of the intended myproject/.babelrc, which is exactly what https://babeljs.io/docs/en/babelrc.html#lookup-behavior describes:

Babel will look for a .babelrc in the current directory of the file being transpiled. If one does not exist, it will travel up the directory tree until it finds either a .babelrc, or a package.json with a "babel": {} hash within.

And that's exactly what's happening to me, because what you refer to as the "current package" is not react-waypoint, it's my app. react-waypoint is complaining that babel-preset-airbnb isn't installed:

 error  in ./node_modules/react-waypoint/es/index.js

Module build failed (from ./node_modules/babel-loader/lib/index.js):
Error: Couldn't find preset "airbnb" relative to directory "/home/dorzechowski/repos/ui-react/node_modules/react-waypoint"
    at /home/dorzechowski/repos/ui-react/node_modules/babel-core/lib/transformation/file/options/option-manager.js:293:19
    at Array.map (<anonymous>)
    etc. etc. etc. etc.

...which is true; I don't have that preset installed. That preset, I'm assuming, is needed to build react-waypoint for use in whatever runtimes it supports. No argument there. But It's not needed to transpile react-waypoint code for use on the targets supported by my app. And indeed, removing node_modules/react-waypoint/.babelrc allows transpilation to succeed, and a smoke test shows react-waypoint behaving normally.

I hope you'll at least consider re-opening this PR; it's awesome that you're publishing this package with a module field and providing ES2015 source files, but kind of a bummer that I have to install otherwise-unneeded babel things to consume those files.

(and either way, thanks for the good work on this lib - we use it in a bunch of places and it's really come in handy)

cheers

@MatthewHerbst MatthewHerbst reopened this Aug 3, 2018
@MatthewHerbst
Copy link
Contributor

Sorry, I did mistakenly think you had made the commit to .gitignore. I deeply apologize for my haste.

That preset, I'm assuming, is needed to build react-waypoint for use in whatever runtimes it supports. No argument there. But It's not needed to transpile react-waypoint code for use on the targets supported by my app.

Would making this change potentially impact users who do need it to transpile for all those other targets? I wonder if the real issue might be that we are missing a peerDependency?

@orzechowskid
Copy link
Contributor Author

orzechowskid commented Aug 3, 2018

no worries! sorry then for explaining to you a bunch of stuff you probably already knew :)

I think that for other projects to successfully transpile node_modules/react-waypoint/es/index.js , they must already be using babel-preset-airbnb and babel-plugin-transform-react-remove-prop-types in their own babel config or else they'd encounter the same error I described above. So they're already set up appropriately, which means there's no impact, and merging this would not be a breaking change requiring a major version bump.

What are you thinking about as a peerDependency, and to which package do you see it being applied? I can't see what a peerDependency would do for anyone here that the existing devDependency entries don't. That plugin and preset are correctly listed as devDependencies of react-waypoint since your source is ES2015+ and your output looks like ES5.

@realityking
Copy link
Contributor

For what it's worth, I agree with adding .babelrc to .gitignore. This package contains the already babelified code anyway, so there's no need to impose this package's babel settings on downstream users who might want to do additional processing.

Copy link
Collaborator

@trotzig trotzig left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me. Thanks!

@trotzig trotzig merged commit 99237f7 into civiccc:master Nov 18, 2018
@trotzig
Copy link
Collaborator

trotzig commented Nov 20, 2018

This PR was included in the 8.1.0 release.

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.

4 participants