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

feat: expose loader context and virtual write to preprocess #144

Closed
wants to merge 1 commit into from

Conversation

jquense
Copy link

@jquense jquense commented Dec 1, 2020

👋 Hey folks. Not trying to be presumptuous, but thought this proposal would be best served with a PR.

This allows the preprocess option to be a function that is passed the webpack loader context as well as the virtual writeModule method. I don't expect this to be commonly used, but allows for more advanced integration into the webpack build pipeline by preprocessors.

The impetus for this is trying to add better css-module (and related languages) support for svelte style tags, but this applies to any preprocessor that might need to do further downstream processing of the resulting styles, markup or js. In my case I need to do further coordination with style files after they have been extracted from the svelte file. Basically this requires doing a very similar trick to what svelte-loader does, by removing the style markup, emitting a (non .css) file and adding an import to that file to the script portion of the component. This bypasses svelte's styling handling but lets webpack process the file further with additional loaders.

This is helpful for few reasons:

  • you don't have to return css that is parsable by svelte's style parser
  • you don't have to awkwardly target multiple file extensions in loaders e.g. .my-css, and .svelte.css

Alternatives to this involve sharing some manner of stateful compiler instance that is passed to both the specific svelte preprocess function and a loader config.

I realize this use-case is hard to articulate, but if it helps there is some prior art for this sort of thing with postcss-loader: https://github.com/webpack-contrib/postcss-loader#function I can also point to my local experiments adding support for my css-module light preprocessor if that might help clarify the need.

@benmccann
Copy link
Member

We removed the virtual modules in #151 so I suspect this PR won't work anymore

@jquense
Copy link
Author

jquense commented Jan 15, 2021

Looks like it just moved to a dependency. I can update this to match the new code if it seems like something y
all would merge

@benmccann
Copy link
Member

I don't think it was moved to a dependency but was actually removed. I'm not a webpack expert though. It's hard for me to give you an answer as to whether we'd accept this because I'm not really sure what it'd look like in the new world. This PR is mostly touching writeModule, which I think doesn't exist anymore, so I don't really have an idea of what a new PR would look like

@jquense
Copy link
Author

jquense commented Jan 16, 2021

O I see @non25 you did the !=! inline thing to add the resource. Hmm I think i'm going to have larger problems than needing an API here then, this syntax have odd sharp edges. Have you gotten this to work with css-loader? My testing of the !=! results in a module without a resourcePath when subsequent loaders process it, which causes them to break since almost every loader expects resourcePath to exist.

@trash-and-fire
Copy link

trash-and-fire commented Jan 17, 2021

@jquense We also ran into problems when using subsequent css loaders, but the solution to the problem is to add a link to the existing file at the end of the import. For example, to the same source file.

import './App.svelte.css!=!svelte-loader/styles?1!./App.svelte';

It's already done in svelte-loader. So there are no problems with subsequent css-loaders.

@non25
Copy link
Contributor

non25 commented Jan 17, 2021

It's not like we've tested it in all scenarios where published svelte-loader was working, and even in those where it didn't work at all. 🤷‍♂️
https://github.com/non25/webpack-svelte-tests
Test for yourself too.

I've tested published svelte-loader with cache-loader, it doesn't work. It is not working with thread-loader due to Virtual Modules too.
That's SFC's virtual css for you.

The way to make svelte-loader work with thread-loader would be to pass css as base64 string in a webpack query and somehow supress the output of such queries in webpack console not to annoy users. That way thread-loader would have access to the css data.

@non25
Copy link
Contributor

non25 commented Jan 17, 2021

@jquense
I've tried thread-loader with query base64 string containing css and it works.
Maybe you want to test this too.
https://github.com/non25/svelte-loader/tree/emitcss-inline
https://github.com/non25/webpack-svelte-tests/tree/thread-loader-tests

It also works with cache-loader put on top of thread-loader.
https://github.com/non25/webpack-svelte-tests/tree/webpack-4-more-loaders

@benmccann
Copy link
Member

I'm most concerned about @jquense's suggestion that postcss or sass imports might not work (webpack/webpack#11074 (comment)). Has anyone tested either of those with the code in master?

I'm less worried about thread-loader since it doesn't work now, it's purely optional, and [the first result on Google suggests it doesn't help much if at all])https://blog.johnnyreilly.com/2018/12/you-might-not-need-thread-loader.html)

@non25
Copy link
Contributor

non25 commented Jan 17, 2021

I'm most concerned about @jquense's suggestion that postcss or sass imports might not work (webpack/webpack#11074 (comment)). Has anyone tested either of those with the code in master?

I'm using sass in my project with prependData. It compiles just fine. Also sass & postcss imports don't even use the approach that is currently in the master. Preprocess runs before it.
We've already discussed that the abscense of resourcePath in his attempts was due to the lack of final path at the end as described in here:

import './App.svelte.css!=!svelte-loader/styles?1!./App.svelte';

If thread-loader can run, then everything can. It is the most restricted environment.

It will defenitily help when using tailwind through @apply in tons of svelte components.
Generating tailwind's AST per component is expensive.

@jquense
Copy link
Author

jquense commented Jan 17, 2021

I trust ya'll have tested the approach I was more curious how y'all solved the issues I had run into trying this in another project. I guess one other downside now is that it's harder for other files to import the extracted css since it's not a file path anymore. Admittedly that is pretty dang unlikely in svelte's context, I don't see why some other file would want to import '/foo.svelte.css' but if people where it'd be much harder now.

@non25 last question, when appending the resource './App.svelte.css!=!svelte-loader/styles?1!./App.svelte' you might still run into issues for downstream loaders that are checking file extensions? This isn't a big deal ATM, but if you wanted to allow the loader to emit .module.css for css modules (or the more obscure .icss.css), css loader's logic for automatically handling these files correctly would likely fail because it checks the resourcePath extension

@non25
Copy link
Contributor

non25 commented Jan 17, 2021

I think then we would have to hack webpack's compiler like in webpack-virtual-modules, but just to trick it to believe that some path exist and actually pass App.svelte.css as final resourcePath instead.
It checks if file exists.

@trash-and-fire
Copy link

trash-and-fire commented Jan 17, 2021

I've just tried with following css pipeline:

      {
        test: /\.css$/,
        use: [
          prod ? MiniCssExtractPlugin.loader : 'style-loader', {
            loader: 'css-loader',
            options: {
              import: true
            }
          }
        ]
      },

I've tried to use following import:

<style>
 @import 'imported.css';
 .test {
   border: 1px solid #ddd;
   padding: 15px;
 }
</style>

It works on first build, but doesn't work on watch-rebuild.

Module build failed (from ./node_modules/css-loader/dist/cjs.js):
Error: PostCSS received undefined instead of CSS string

We should answer the question: "Why do we want to process css files through the webpack css pipeline when we have svelte-preprocess?" For example, if the imported file contains classes, they will not be hashed by the Svelte compiler if the file is processed by the webpack pipeline.

It seems that css processing via webpack only has one purpose: it should be merged into one file with the other pieces of css without any additional processing or with minimal additional processing.

@non25
Copy link
Contributor

non25 commented Jan 17, 2021

Maybe we should change or make an option to change output extension ?
For example I would only use autoprefixer on component's css.
Or, edit regexp that matches .css not to match .svelte.css. That would fix it.

@trash-and-fire
Copy link

Maybe we should look into the vue-loader approach, but it's a much more complex SFC loader.

@benmccann
Copy link
Member

Why do we want to process css files through the webpack css pipeline when we have svelte-preprocess?

The other use case that comes to mind for me is css code splitting, but that's similar in that it's just dividing up how css is put in files and not actually changing the contents

@jquense
Copy link
Author

jquense commented Jan 18, 2021

Why do we want to process css files through the webpack css pipeline when we have svelte-preprocess?

There are few reasons to prefer webpack's pipeline:

  • you only have to configure stuff once, reducing the chance svelte's css is handled slightly different
  • webpack has larger existing ecosystem than svelte (for now), being able to reuse tooling without svelte specific caveats or hooks is a win.
  • App wide optimizations are only possible from webpack's perspective. CSS modules enables similar optimizations to svelte CSS (removing unused css, etc) but cross file, and between component and non-component code.

The last one is the space I've been exploring in particular with my own CSS preprocessor. I originally wanted this PR to completely opt out of svelte's css handling by removing the style and emitting the uncompiled style for webpack to handle and optimize via another loader. That's a bit here nor there, but generally is the svelte-preprocess model precludes more interesting cross file compilation optimizations for non-JS languages since you need to compile away those languages before the tool with dep graph knowledge can see them. I'll just say that this is totally not a common use-case, i'm actively experimenting here, so was looking for reasonable escape hatches, not first class support (yet).

it does seem tho like since the VM's were removed there isn't much to add here. It may make sense to expose some option to adjust the file extension in the future but i can hack that in and see if it helps for my experiments.

@non25
Copy link
Contributor

non25 commented Jan 18, 2021

I feel like if you prefer webpack's pipeline and want to use css modules you would be better off opting out of SFC.
If you still want to use SFCs however then look at https://www.npmjs.com/package/@modular-css/svelte

I don't see much of a choice for general svelte usage, since webpack-virtual-modules are in wierd state too.
You can adapt #146 to the new code and make your version of svelte-loader, which fits your needs. It is not hard.

As to making consistent configurations for external css and svelte component css... I see two steps for css:

  1. preprocess (sass-like syntax extensions, postcss utils, etc.)
  2. postprocess (autoprefixer, pxtorem, etc.)

So ideally, for svelte, you would want to make mergable preprocess and postprocess parts.
Mix them together for global css, and separate them, putting pre to svelte-preprocess and post in .svelte.css webpack's pipeline.

What problems these aren't solving ?

webpack has larger existing ecosystem than svelte (for now), being able to reuse tooling without svelte specific caveats or hooks is a win.

Can you make an example ? What tooling you are having problems with ?

App wide optimizations are only possible from webpack's perspective. CSS modules enables similar optimizations to svelte CSS (removing unused css, etc) but cross file, and between component and non-component code.

I don't see how this fits svelte's scoping mindset.

As to last paragraph I don't understand what you are getting at. What problems you are trying to solve?

To conclude, svelte-loader should be made good for general svelte usage. It should be reliable, and depending on webpack-virtual-modules in its current implementation is less attractive than the solution we have now in the master.

@jquense
Copy link
Author

jquense commented Jan 18, 2021

To be clear i'm not advocating for using virtual modules, this PR was an attempt to simply add an escape hatch, and since the underlying approach changed its not really relevant any more. I think we're veering into a more general discussion about pain-points of using compiled style languages with svelte, and downsides with svelte's css scoping approach. I can open a separate issue about that with more clearly stated issues. In the meantime I'm fine forking svelte-loader if needed (that's where this came from) especially since i'm not the general use-case.

@jquense jquense closed this Jan 18, 2021
@jquense jquense deleted the expose-write-file branch January 18, 2021 20:42
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.

None yet

4 participants