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

Bundle CSS file if CSS is not injected to DOM #11

Merged
merged 1 commit into from
Apr 13, 2018

Conversation

akihikodaki
Copy link
Contributor

@akihikodaki akihikodaki commented Apr 12, 2018

VueAsset does this, so why doesn't SvelteAsset?

Some design questions:

  • Shouldn't compilerOptions.css be false by default? This would be a breaking change, of course. However, it would bring consistency with CSSAsset and VueAsset.
  • It uses CSSAsset to post-transform CSS, but is it really fine? It already has preprocess to transform CSS, and this feature is kind of redundant. I decided to use the class, however, because it already employs JSAsset to transform JavaScript output, and the CSSAsset brings an important feature: to resolve url(./foo.png).

@akihikodaki akihikodaki changed the title Bundle CSS file if CSS if not injected to DOM Bundle CSS file if CSS is not injected to DOM Apr 12, 2018
@DeMoorJasper
Copy link
Owner

DeMoorJasper commented Apr 12, 2018

Thank you so much for the PR!

I think it should be fine to just do it the same as vue without having the internal css parsing? What do you think? (removing CSSAsset in here)
Not sure why anyone would inline css, as in the future it's gonna be slower than external anyway (and currently doesn't make a big diff)

@akihikodaki
Copy link
Contributor Author

akihikodaki commented Apr 12, 2018

I think it should be fine to just do it the same as vue without having the internal css parsing? What do you think? (removing CSSAsset in here)

I decided to use CSSAsset because Svelte lacks a feature to resolve something like url(./foo.png). It is too hard to implement with preprocess. (Indeed I have no idea other than adding more dirty hacks and boilerplate.) (UPDATE: of course I can change the behavior if you decide not to use it.)

Not sure why anyone would inline css, as in the future it's gonna be slower than external anyway (and currently doesn't make a big diff)

So you want compilerOptions.css to be false by default? I will make a change accordingly if so. (Note that setting false to compilerOptions.css will put CSS in a separate file.)

@DeMoorJasper
Copy link
Owner

DeMoorJasper commented Apr 12, 2018

@akihikodaki I only proposed that if it would get rid of CSSAsset if it doesn't than it doesn't really matter.

@akihikodaki
Copy link
Contributor Author

@DeMoorJasper Well, I'm having a bit difficulty to understand your comment. You mean url(./foo.png) does not really matter and CSSAsset should be removed?

@DeMoorJasper
Copy link
Owner

Ow sorry, I meant if putting css in a seperate file gets rid of CSSAsset i would've preferred it, but apparently it doesn't so the default value doesn't really matter now.

Basically meaning leave the PR as is, unless u can fix the CSSAsset hack fix :)

@akihikodaki
Copy link
Contributor Author

I understand. in the current implementation, CSSAsset is used only when putting CSS in a separate file, though it is always instantiated.

CSSAsset is used not because it is necessary to put in a separate file, but it is to transform:

.main {
  /* Reference an image file */
  background: url('./images/background.png');
  color: red;
}

(copied from https://parceljs.org)

So I'm wondering if you think this CSS should be supported.

@DeMoorJasper
Copy link
Owner

Yes it should, kinda surprised svelte doesn't automatically transform css requires into js requires if it inlines it.

I'll merge, possibly improve and release this soonish

@DeMoorJasper DeMoorJasper merged commit 22c761b into DeMoorJasper:master Apr 13, 2018
0x130C added a commit to 0x130C/parcel-plugin-svelte that referenced this pull request Apr 16, 2018
* Bundle CSS file if CSS is not injected to DOM (DeMoorJasper#11)

* add test environment

* 1.0.0
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