reworkcss / rework-npm

Import CSS from npm modules using rework
BSD 2-Clause "Simplified" License
87 stars 19 forks source link

Add shim option for packages that don't define a style field or use index.css #2

Closed bclinkinbeard closed 10 years ago

bclinkinbeard commented 10 years ago

This adds the ability to provide a shim config option for packages that neither specify a style field in their package.json nor provide their styles as index.css. The shim option is expected to be a hash, where the key is the package name and the value is the path to the CSS file. The only caveat is that the shimmed package must have a package.json file, but that seems like a safe assumption.

I added a test to verify the behavior, and if you're good with the change I can also update the README prior to merging.

conradz commented 10 years ago

You can do @import "my-module/my-file.css"; for files that don't have index.css or a style field. However, this could definitely be useful. Shouldn't the shim config override the style field, even if it is defined?

bclinkinbeard commented 10 years ago

You can do @import "my-module/my-file.css"; for files that don't have index.css or a style field.

I did not know that. :) You think there is still value in this though?

Shouldn't the shim config override the style field, even if it is defined?

You're probably right. I can make that change.

conradz commented 10 years ago

I personally don't need this, but I'll let others decide. I guess it could be nice to be able to import a package by package name instead of the path, but I have never ran into this situation. Usually you could just make a PR that adds style to the package.json for a package that is on NPM.

If anyone has a use for this, I'm open to adding it.

bclinkinbeard commented 10 years ago

I think that is a valid point. I'd rather configure the path in my build file or wherever, rather than in my CSS file(s). As for the PR approach, I recently did exactly that, but the project owner did/does not want to publish to npm before the next official release. Judging from the project's history that could easily be more than a month away.

Maybe @Techwraith wants to chime in here, but given those things I think it would be nice to add this option. I will update the PR tonight to prefer shim over package.style and will update the README as well.

techwraith commented 10 years ago

I think this is a good transition enabling feature. For people who are moving their app over to a modular css app from a monolith, this will help a ton. It'll allow people to use css by name that they don't have in their node_modules directory.

conradz commented 10 years ago

@Techwraith I don't think this is for naming packages that are not in node_modules (at least I think that's what you're saying), but for node_modules packages that don't have the style property in package.json and also don't have a index.css file.

For example, if I have a module my-styles in node_modules/my-styles that contains a styles.css file and a package.json file, with this change I could pass { 'my-styles': 'styles.css' } in the shim option and then @import "my-styles"; which would then import node_modules/my-styles/styles.css.

conradz commented 10 years ago

@bclinkinbeard I can definitely see that shim would be useful in that case, so as soon as it's updated with the changes I'll merge it in.

techwraith commented 10 years ago

Ah, I didn't look thoroughly enough. I think this would be better if you could just specify any path to shim with a name.

{'my-app-base': './path/to/my-app-base.css'}

and then you could import ./path/to/my-app-base.css by doing:

@import "my-app-base";

If you wanted to do this for something in node modules it would be like this:

{'my-app-base': './node_modules/my-app-base/my-css.css'}
bclinkinbeard commented 10 years ago

Yea, this is specifically for packages that are not yet following the package.style/index.css convention. I could see the benefit of specifying any path as well though, and would be willing to do the implementation. At the risk of overcomplicating things, I think that would belong in an alias config option though. That would keep things pretty well aligned with Browserify, where you shim packages/files that don't adhere to the system, and alias files that just have annoying path.

Thoughts?

techwraith commented 10 years ago

Yeah, that's fine with me.

bclinkinbeard commented 10 years ago

I think this is all set. Test fixtures added, shim prioritized, README updated.

conradz commented 10 years ago

This landed in the v0.4.0 release! Thanks for the contribution!

bclinkinbeard commented 10 years ago

Awesome, thanks! We'll get a new version of atomify-css out soon with the updated dependency.