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

Update .npmignore to no longer exclude src/ #165

Closed
wants to merge 1 commit into from
Closed

Update .npmignore to no longer exclude src/ #165

wants to merge 1 commit into from

Conversation

EdwardDrapkin
Copy link

If you try to use react-css-modules with the babel-modern preset, one of the transforms used in the distribution process is incompatible.

This is probably an edge case, but I want to include the source directly, let my babel loader know about it, and then use the babel-modern preset.

Can you start distributing the source folder in npm packages please? Thanks!

@gajus
Copy link
Owner

gajus commented Aug 19, 2016

If you try to use react-css-modules with the babel-modern preset, one of the transforms used in the distribution process is incompatible.

What are you referring to?

@gajus
Copy link
Owner

gajus commented Aug 19, 2016

Can you start distributing the source folder in npm packages please? Thanks!

This would require you to rely on relative paths to include the source, i.e. require('react-css-modules/src'). This in turn would make the file system dependant of the versioning. This is not something I would want. Furthermore, it would include the download size for everyone else.

However, you/I/someone could just publish react-css-modules-es6 package.

@EdwardDrapkin
Copy link
Author

EdwardDrapkin commented Aug 20, 2016

Do you have experience bundling multiple NPM packages out of the same repository? I'll be glad to setup the scripts for you and send a pull request, but this is an area where I have no experience. If you want something particular done, let me know, otherwise on Monday I'll start looking at what other packages' approaches might be.

Also can we call it 'react-es6-css-modules' because conceptually the difference is "css modules" vs "es6 css modules"?

@gajus
Copy link
Owner

gajus commented Aug 20, 2016

Do you have experience bundling multiple NPM packages out of the same repository? I'll be glad to setup the scripts for you and send a pull request, but this is an area where I have no experience. If you want something particular done, let me know, otherwise on Monday I'll start looking at what other packages' approaches might be.

What I don't understand is – suppose we distribute the ./src as it is. How would your script know which transformations need to be applied to make it work? Would you hardcode a config for a babel loader specific to react-css-modules?

@EdwardDrapkin
Copy link
Author

EdwardDrapkin commented Aug 20, 2016

I just use the same babel-loader as the rest of my files:

    babel: {
        happy: {id: 'babel'},
        test: /\.jsx?$/,
        exclude: /(?:(?:node_modules|bower_components)(?!\/(?:react-css-modules))|(?:__tests__))/,
        loader: `babel-loader?cacheDirectory=.babelcache/${process.env.NODE_ENV}/`
    },

Since you are also omitting your .babelrc from the distribution folder, my babel transform picks them up.

@EdwardDrapkin
Copy link
Author

EdwardDrapkin commented Aug 20, 2016

Alternatively, you could bundle files with a separate babel transform chain for es6 module based projects, like react-router does (they provide e.g. both react-router/es6/Link and require('react-router').Link if you want ES6 or CommonJS modules), which is what I was going to figure out how to set up since you don't want to distribute source files.

@gajus
Copy link
Owner

gajus commented Aug 20, 2016

I just use the same babel-loader as the rest of my files:

The fact that it is working, is coincidental. This is not a reliable way to import/ use a dependency.

@EdwardDrapkin
Copy link
Author

As long as there's no .babelrc, the module resolver will resolve 'react-css-modules' as the installed version at node_modules, so import react-css-modules/src/index is going to resolve to the correct file. Since the webpack config isn't importing from the src directory, it doesn't need to apply babel transforms. How is this unreliable, except for if you start shipping .babelrc?

@gajus
Copy link
Owner

gajus commented Aug 20, 2016

How is this unreliable

What happens when I (as an example) add Flow types to the codebase? Your build breaks.

Maybe I am misunderstanding the proposal.

Are you suggesting to distribute the source code as it is, or are you suggesting to distribute a source code that omits the babel-plugin-transform-es2015-modules-commonjs transformation?

@EdwardDrapkin
Copy link
Author

EdwardDrapkin commented Aug 20, 2016

I get what you're saying, but it's a risk I'm willing to take.

The specific transform that's breaking for me is (I think) es2015-classes because I'm trying to use the modern-browsers transform (which omits it) because it's noticeably faster for compile times for development. What I'm doing is kind of esoteric, and I understand it's risky, but it's something I'm willing to do because it's one of the things I have to do to keep incremental build steps under a second. I understand that it's not best practices and it incurs a chance of everything breaking, but I'm willing to accept those risks because it's strictly something I'm doing during development (I'm still using the standard es2015 transform in my production bundle), and worst case scenario is I have to change a single line in my .babelrc to bail out.

I don't think the additional cost of a few extra KB of download per npm install is really a reason not to distribute the source folder. If you don't want to offer support for people doing crazy things like this, I get it, but why not just let me get at the source folder at my own risk? If you're dead set against it, I can setup a separate distributed 'es6' file the same way that react-router does, but that seems like an awful lot of work, since the primary benefit of distributing things that way is bundle size.

@gajus
Copy link
Owner

gajus commented Aug 20, 2016

why not just let me get at the source folder at my own risk?

A story of how NODE_PATH is still being supported today, although it was never designed to be used outside of a very esoteric use case, is a good example.

nodejs/node#1627

why not just let me get at the source folder at my own risk?

Given this unique requirement, why not simply:

git clone [email protected]:gajus/react-css-modules.git
cd ./react-css-modules
npm link

and you are set to use react-css-modules/src in your code base?

Sure, this would mean needing to pull the latest changes every time a new version is released. However, as you said, this is used only for an esoteric use case and only for development purposes.

@EdwardDrapkin
Copy link
Author

EdwardDrapkin commented Aug 20, 2016

These are all risks I'm willing to take. It seems like you're trying to protect me from myself, which I don't think is necessary.

I don't want to rely on git versions because it makes shrinkwrapping a bit more difficult.

We could state this issue a different way:

I want to use react-css-modules with only the modern-browsers transform, since I don't need ES2015 support. I can't do that. I don't think it's fair to force users to support old browsers if they don't want to, especially if the only cost is a few KB of node_modules cruft, which won't get noticed. There's already packages that distribute ASCII art, their test files including an entire novel, etc. so what's a few more KB that no one is going to care about?

The easiest fix (for now) is to just distribute the src/ folder as is, and let the esoteric users figure it out. When (as I believe will be sooner rather than later) the JS community writes tools like UglifyJS that work with ES6, I think this sort of requirement will move from esoteric into common place.

@gajus
Copy link
Owner

gajus commented Aug 20, 2016

I don't want to rely on git versions because it makes shrinkwrapping a bit more difficult.

You don't rely on git versions. You'd be using this only for development, as you have said.

I can't do that.

You can, just branch out and release a package that supports only the modern browsers. It is a valid request, just not popular enough to be included in the main repository.

When (as I believe will be sooner rather than later) the JS community writes tools like UglifyJS that work with ES6, I think this sort of requirement will move from esoteric into common place.

And then it will be included in the main package.

@gajus
Copy link
Owner

gajus commented Aug 20, 2016

This is starting to go roundabout.

To summarise:

  • Uncompiled version of react-css-modules is not going to be included in the package. This can change in future if it becomes a popular demand.
  • You can use npm link for development purposes.
  • You can branch out and release a version of react-css-modules that distributes uncompiled code.

@gajus gajus closed this Aug 20, 2016
@gajus
Copy link
Owner

gajus commented Aug 20, 2016

Thank you for the PR.

@EdwardDrapkin
Copy link
Author

EdwardDrapkin commented Aug 20, 2016

You don't rely on git versions. You'd be using this only for development, as you have said.

I do have to. What your suggesting requires me to keep track of what git version corresponds to what NPM version and keep the two in lock step.

I don't want to sound rude, but all of your suggestions incur a significant time and technical cost on my part, and the only argument against including the source folder is NPM distribution size when I've already provided a pull request. Not including the source folder in NPM distribution isn't even considered best practice, so why are you so dead set against it?

I don't understand your apprehension in including the source folder at all. Does saving 36K per download really justify risking alienating your users or a fork? There's literally zero cost to you to merge this pull request and you're denying it... why?

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.

2 participants