Bug: unable to inject multiple times when auto-inject is off

[dominykas]

Introduced in #24: https://github.com/davidguttman/cssify/commit/d8d275354a99f906974506226adf0cae937990a8#diff-168726dbe96b3ce427e7fedce31bb0bcR27

When injecting the second time, the filename/ID is undefined again and overrides the already injected content. I think the correct fix would be to not set style.id when filename is not a non-empty string?

/cc @faergeek

[faergeek]

Sorry, I didn't understand the issue.

How do styles inject if auto-inject is off?

[dominykas]

I'm talking about browserifying the CSS file with options['auto-inject'] and then calling cssify on the browser side manually. The filename parameter now becomes required. Oh. I looked at the wrong line :) Anyways, is it required? Should it be required? I mean it's not a problem for me to generate something random on the client side, but maybe the ID is not necessary, just as before?

[faergeek]

Thanks, now I understand, you just want to not set id if it's undefined, right?

[faergeek]

I just didn't think about such case when you can do it manually

[dominykas]

I mean it's fair enough - it's a major version (well, introduced pre-V1) increment, so breaking changes are OK, however it does seem a little uncomfortable to have a required param (fileName) after an optional one (customDocument), and it's easy enough to handle the undefined fileName. I can PR later this week, if you're busy.

[faergeek]

Would be cool if you submit PR. It's important to not break HMR-related functionality as there are no tests for it yet.

[faergeek]

Question not related to this issue: do you extract styles into a separate stylesheet somehow and if not would you like to have this possibility implemented somehow? :-)

[dominykas]

Not sure I understand your question... Either way, my use case is injecting CSS into a dynamically constructed iframe.

[faergeek]

I mean that for now there's no easy way to extract styles into a separate css file, all styles are stored directly in JS, so if JS is not included in head you'll see unstyled page for a bit (depending on your connection speed), I think it's not good for production.

[faergeek]

Also, if JS is disabled you'll never see the styled page, pretty rare case in these days probably, but anyway.

[faergeek]

Take a look at #21

[dominykas]

Oh, OK. This sounds like a useful thing when building isomorphic apps, but not relevant for my current use case, since there's no "server side" [and can't possibly be] in what I'm building (a widget embeddable on a 3rd party site).

[faergeek]

I understand, that's because I said that it's not related to this issue, just a workflow question ;-)

[dominykas]

However, I'm not sure that should be within the scope of cssify itself, though... But once again - this is my use case and my thinking. Essentially, I have one or more less files and I'm including those and LESS/Sass can handle bundling itself and output a concatenated file somewhere for use with code coming from the server side anyways - I'm a fan of small modules doing one thing well yada yada :)

[dominykas]

Hmm. If you're not using less then yeah, having one CSS file with all that was required is probably nice... But once again - in my use case, to optimize downloads, I have 3 browserify bundles for JS, which all include some CSS and there's also a base CSS file in the "seed" bundle too, so it wouldn't be as trivial as "generating one concatenated CSS file" - there's essentially multiple CSS bundles in each JS bundle and there's some modularity involved.

[faergeek]

Small modules doing one thing is cool that's why I migrate to browserify. I'm not going to generate concatenated css using cssify itself, I want to use existing partitioning solutions instead, so we need to provide possibility to integrate with them somehow

If you have experience with partitioning then welcome to #21 :-)

[faergeek]

@dominykas Can you test if my PR really fixes your issue?

[dominykas]

Sorry, my schedule has been wrecked a little with a brand new family member :) But I promise I'll get back to this asap, although it will probably be hard to do before xmas.

[dominykas]

Bit late to the party... but all is goo now ;)