Skip to content

Skip non-matching insert-css statements #3

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

Closed
wants to merge 1 commit into from

Conversation

ajoslin
Copy link
Contributor

@ajoslin ajoslin commented Mar 22, 2016

This allows people to still use require('insert-css') in their code.
If it doesn't match the expected format, it just won't be extracted from the javascript bundle.

Before, non-matching insert-css statements caused an error.

@codecov-io
Copy link

Current coverage is 100.00%

Merging #3 into master will not affect coverage as of e099750

@@            master      #3   diff @@
======================================
  Files            1       1       
  Stmts           35      36     +1
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit             35      36     +1
  Partial          0       0       
  Missed           0       0       

Review entire Coverage Diff as of e099750

Powered by Codecov. Updated on successful CI builds.

This allows people to still use `require('insert-css')` in their code.
if it doesn't match the expected format, it just won't be extracted from
the javascript bundle.
@ajoslin ajoslin force-pushed the manual-insert-css branch from cf4cbae to 3951399 Compare March 22, 2016 16:51
@ahdinosaur
Copy link
Member

ooh, looks good to me. 😄

merged as fbb0963, @yoshuawuyts can we has publish?

@yoshuawuyts
Copy link
Contributor

Actually wait a lil bit. I'm not sure this is desirable, as something cool about css-extract is that it can extract all insert-css calls and bundle them together - allowing interop with any css tooling as long as they use insert-css.

@ajoslin you mentioned errors occurred? Could you explain what was going wrong?

@joshwnj I know you were thinking of using this for css-modulesify, would this patch complicate things for you?

I def like the implementation and idea, but just being cautious that we won't have to revert this change down the line

@ajoslin
Copy link
Contributor Author

ajoslin commented Mar 23, 2016

@yoshuawuyts My specific use-case is inserting a base64 font that I fs.read with brfs at compile time.

The current errors, however, occur with anything that simply requires and calls insert-css on separate lines:

var insertCss = require('insert-css')

insertCss('.foo { color: red }')

The current parsing only works for the case of a require and its usage inline:

require('insert-css')('.foo { color: red }')

With the current ast-parsing logic, there's no way to cover every case (eg we can't cover insertCss being passed into a function in a different file). but I think that's OK.

We could definitely improve the logic to know how to read these non-inline cases. Similar to brfs, we don't need to cover everything.

I'm not very familiar with ast parsing, so I don't know how reliable I would be at expanding the logic here.

@joshwnj
Copy link

joshwnj commented Mar 24, 2016

@yoshuawuyts I don't think this patch will complicate things for me - cheers

@ahdinosaur
Copy link
Member

The current parsing only works for the case of a require and its usage inline:

ah. hmm, could we use static-module here?

@ajoslin
Copy link
Contributor Author

ajoslin commented Mar 24, 2016

Yes, static module is exactly what we need here! That makes things extremely simple.

The css-extract plugin can just add a static-module transform for insert-css to the bundle.

The only confusion here would be that dynamic uses of insert-css won't get found by static-module. But I think that's OK if we document it.

I'll open a PR when I can. I say we close this for now in favor of a much better static-module implementation.

@ajoslin ajoslin closed this Mar 24, 2016
ahdinosaur added a commit that referenced this pull request Mar 24, 2016
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.

5 participants