Updates to make watchify work

[joshwnj]

• using a different kind of tranform that ends the stream correctly
• inject require statements for composed css modules, so that they are
  added to browserify's dependency graph
• clear token cache so that we can rebuild the parts that change

[snikch]

Just moving my comment in here from #63 since they were in the wrong place.

This PR works for any new changes, but there appears to be a cache which causes changes it has previously rendered, to not cause an update.

Hrm, I spoke too soon. It will handle a change which it hasn't seen before, but making a change then reverting it will not result in an update.

For example, running with a padding: 10px, then changing to padding: 60px will correctly rebuild the css, but changing back to padding: 10px will not. Updating to a new value, such as padding: 600px, will trigger an update.

[snikch]

It appears to be a bug with dedupeSources. If I comment out delete sources[key] at Line 77, I correctly get changes applied each time.

[joeybaker]

@joshwnj did you consider through2? It might be easier than writing your own stream implementation.

[snikch]

Another issue, this doesn't appear to be picking up changes in files that are imported via composes.

[joshwnj]

@snikch yeah I noticed that one too :| Will check the dedupeSources thing, thanks. It's possible that with the update to css-modules-loader-core we won't have any chance of duplicate sources anyhow.

I'll look closer at the composes issue you noted, because I thought that this was one thing I had managed to solve :P (with this addition: https://github.com/css-modules/css-modulesify/pull/64/files#diff-168726dbe96b3ce427e7fedce31bb0bcR183)

Would you mind sending me a gist or something of the case you're seeing it in?

[joshwnj]

@joeybaker I'll take a closer look at through2, I think I tried it but must confess it wasn't the most thorough trial :) When I found an approach that worked properly with module-deps (which is actually borrowed from https://github.com/babel/babelify/blob/master/index.js) and allowed us to inject new requires with a transform that get picked up as new dependencies, I was happy to move on :)

Definitely though let's revisit this once we have it working and see if a cleaner solution is available.

[joshwnj]

Got a few minutes to look at this now, here's what I'm seeing:

• looks like we won't need deduping anymore (this will be done in css-modules-loader-core)
• I think I've reproduced the composes bug @snikch mentioned (i was making a bad assumption in my diff algorithm to find dependencies)

Will have a crack at fixing this now

[joshwnj]

I've made some updates in and I think we're nearing a solution. I've removed deduping and used a better approach for keeping track of dependencies.

@snikch / @joeybaker / @markuplab / @justgook / @tinchoz49 - if you have a few moments to try out this PR it would really help make sure we've got all of the bases covered. I think it's working well for the main case but want to make sure we cover a wide range. - Thanks

[justgook]

need to try..

[justgook]

Works for me, as expected.. minimal changes in code, and works like charm - as soon as it will be merged will push updates to my repo

classNames = require 'classnames/bind'

styles = require './boxes.css'
cx = classNames.bind styles

[tinchoz49]

hi @joshwnj, yes no problem! thanks for you effort!

(checking...)

[snikch]

Looking really healthy @joshwnj! I can confirm that both of the issues I was facing are resolved 👌. I'll be using it over the next few hours so I'll be back if I hit any snags.

[tinchoz49]

works ok to me! 👍 this is a great project and great idea by the way!

now my problem is the issue 65 :(

[snikch]

Ok, so I've only come across one issue. If you are composing another file and the class doesn't yet exist, then creating it in the composed file won't correctly update. I seem to need to save the file that composes the other again to get the update to show.

Steps to reproduce.

1. Create files a.css and b.css, and import a.css somewhere in your js.
2. In file a.css create a class .a { composes: b from './b.css' }. Save a.css.
3. Any javascript that uses class a will not have the composed styles since they don't exist e.g. _components_a__a undefined.
4. In file b.css create .b { color: #444; } and save b.css.
5. Any javascript that uses class a will not have been updated with the composed styles e.g. they're still _components_a__a undefined.
6. Save a.css again to fire watchify (no actual changes to a.css have been made).
7. Javascript that uses class a will now have been updated with the composed style e.g. _components_a__a _components_b__b.

[joshwnj]

Thanks for the report @snikch - I'm able to see that bug too. I have a solution in mind that I think will fix the problem and also be an overall improvement: if we can intercept the css while it's still in icss form we'll have a really nice bridge between css and js, converting the icss imports/exports into js imports/exports.

Will let you know if I'm able to get that working.

[justgook]

i think about _components_a__a undefined - should be warned as error/warning, and throw some compilation error - that will not solve the problem (about nested compilation), but will reduce lot of problems (in real development live)

[justgook]

about @snikch report - i think it is different story, that can be opened as different bug, because that PR is stopper to use tool in real development.
is it painfull - yes
can we live with it - yes
can we use it without that - yes
can we start develop without PR - no

[joshwnj]

Ok, I'm going to move ahead with this PR and lodge a new issue for dealing with undefined composition, as @justgook suggested. I can still see a way forward here but after looking into it, it seems like it will involve reaching pretty deep into css modules stuff and I'd rather take the time to do it right.

@snikch in the meantime I've made an update so it will display an error message in the case you outlined.

Thanks everyone for your input here

[snikch]

👌 thanks @joshwnj

[joshwnj]

Publised v0.16.0

Closes #63

[joeybaker]

Thanks @joshwnj!