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.

Sign up for GitHub

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

Jump to bottom

{mode: 'extract'} without css modules #117

Closed
dilyanpalauzov opened this issue May 4, 2020 · 12 comments
Closed

{mode: 'extract'} without css modules #117

dilyanpalauzov opened this issue May 4, 2020 · 12 comments
Assignees
@Anidetrix

Comments

@dilyanpalauzov
Copy link

dilyanpalauzov commented May 4, 2020 •
edited

https://github.com/Anidetrix/rollup-plugin-styles/blob/master/README.md says:
“Note that, by default, generated CSS will be injected into <head>, with CSS also available as default export unless CSS Modules are enabled,”... “CSS Modules styles({ modules: true,})”

This means, that CSS Modules are enabled with modules:true and unless the modules are enabled, the css will be injected into head.

It turns out, that style({mode:'extract'}) prevents the automatic injection.

mode:'extract' changes just the file extensionof the entryfilename, but I want to use as name something that contains the hash of the css-content and ideally honours the https://rollupjs.org/guide/en/#outputassetfilenames option.

With style({mode:['extract':'dist/[hash]-e.css']}) the file is called literally [hash]-e.css

With style({mode:['extract':'e.css']}) rollup receives '../e.css' and says the filenames cannot be relative or absolute.

With mode:'emit' I get:
! Error: unexpected token (Note that you need plugins to import files that are not JavaScript)
xxx/zzz.css:
(css code)

• The mode:['extract', 'SECOND-ELEMENT'] shall honour output.dir and be just file without path.
• The generated file shall honour the output.assetFileNames pattern, possibly inserting its own hash in the filename.
• Please elaborate in the documentation how mode:emit is supposed to be used
@Anidetrix Anidetrix self-assigned this May 4, 2020
@Anidetrix
Copy link
Owner

Anidetrix commented May 4, 2020 •
edited

Hello again @dilyanpalauzov, thanks for another contribution!

This means, that CSS Modules are enabled with modules:true and unless the modules are enabled, the css will be injected into head.

My bad, now that I read it again I understand that it is misleading, extract in fact is not meant to inject CSS (and it doesn't), but the other half of the statement about default export is true. I'll change the wording and let you know.

mode:'extract' changes just the file extensionof the entryfilename, but I want to use as name something that contains the hash of the css-content and ideally honours the https://rollupjs.org/guide/en/#outputassetfilenames option.

Yes, I'm aware of that, and currently working on it (in fact the hash augmentation part is already there).

With style({mode:['extract':'e.css']}) rollup receives '../e.css' and says the filenames cannot be relative or absolute.

Yep, this is a bug, thanks for finding this. I'll change it to being relative to output directory (also this is a breaking change, so it will be v3), or I will add an error which warns about resulting path being outside of dist dir if Rollup's dir output option is provided, otherwise handle it relative to current working directory.

With mode:'emit' I get:
! Error: unexpected token (Note that you need plugins to import files that are not JavaScript)
xxx/zzz.css:
(css code)

Please elaborate in the documentation how mode:emit is supposed to be used

It is intended, since this is meant for later plugins in the build pipeline, so it doesn't generate JS wrapper, keeping chunk pure CSS, which makes Rollup angry since it does not understand it. But yeah, this should be better documented, so will do.

Also with that in mind, it is hard to produce a proper example, but i'll think of something.

@Anidetrix
Copy link
Owner

I've released v3, hopefully it fixes all the problems you listed and more, check it out and let me know

@dilyanpalauzov
Copy link
Author

With version 3.0.2, I set output:{dir:'dist', assetFileNames: 't/[name]-[hash][extname]'}, plugins:[styles({mode: ['extract', 'e.css']})} and call rollup -c.

rollup-plugin-styles does find the files referenced by url(relative-paths) in the .css sources and moves them to the output directory (dist).

The resulting .ccs file is correctly located and called dist/t/e-[hash].css .

But the other assets are under the dist/assets/assets folder. Moreover, t/e-[hash].css contains now the relative urls as url(./icon...png) which does not work out of the box, since e.css is in the dist/t folder and the png files are in the dist/assets/assets folder.

@Anidetrix
Copy link
Owner

Anidetrix commented May 5, 2020 •
edited

But the other assets are under the dist/assets/assets folder.

Well, this is kinda intended, because at first i planned for both CSS assets and assets from CSS URLs
to respect assetFileNames, so if you set assetFileNames to [name]-[hash][extname], then CSS would be near the JS files, and URL assets would be in assets directory, but since they are separated you need to set both hash and assetFileNames to the same value to achieve the same effect. Probably should document this better or move/rename this option to something like depFileNames, or even cut assetFileNames support altogether and provide cssFileNames option for both CSS and assets, as it was intended beforehand.

Moreover, t/e-[hash].css contains now the relative urls as url(./icon...png) which does not work out of the box, since e.css is in the dist/t folder and the png files are in the dist/assets/assets folder

I believe either assetDir should be "." by default, or publicPath should be "./assets" by default, but I still thinking about which will suit more use cases.

@Anidetrix
Copy link
Owner

Ok, assetDir = "." it is. I believe now URLs make sense by default.

@dilyanpalauzov
Copy link
Author

Alright. Now, when the output.assetFIlenames is not changed, and stays assets/[name]-[hash][extname] with module: 'export' the css file goes into the {output.dir}assets/ directory and the png files extracted from url()-css also go in the same directory and this works altogether.

But when I change output.assetFIlenames then assets extracted form url()-css do to respect the new output.assetFileNames. This is not a big issue, but would be nice, if the assets coming from rollup-plugin-styles honours output.assetFileNames.

Moreover, the default for output.assetFileNames is "assets/[name]-[hash][extname]", but https://github.com/Anidetrix/rollup-plugin-styles/blob/master/README.md recommends changing it to assetFileNames: "[name]-[hash][extname]". With this change, things stop working out-of-the box, as the url(...png) files go into {output.dir}assets/ folder and the css file generated from rollup-plugin-styles go in the {output.dir}/ folder, references url (./png) and this does not match.

@Anidetrix
Copy link
Owner

But when I change output.assetFIlenames then assets extracted form url()-css do to respect the new output.assetFileNames. This is not a big issue, but would be nice, if the assets coming from rollup-plugin-styles honours output.assetFileNames.

It would be, but as I said this is pratically impossible since assetFileNames is not available at transform build stage, at which URLs are being resolved, so it seems to be either this or swap assetFileNames altogether with custom option.

Moreover, the default for output.assetFileNames is "assets/[name]-[hash][extname]", but https://github.com/Anidetrix/rollup-plugin-styles/blob/master/README.md recommends changing it to assetFileNames: "[name]-[hash][extname]". With this change, things stop working out-of-the box, as the url(...png) files go into {output.dir}assets/ folder and the css file generated from rollup-plugin-styles go in the {output.dir}/ folder, references url (./png) and this does not match.

True, clarified the documentation there, thanks.

@dilyanpalauzov
Copy link
Author

Do I understand you correctly, that the css-url() assets are inserted with this.emitFile(type:'asset', name) during the transform hook and emitFile ignores the output.assetFileNames option when called from.transform()?

If this is a deficiency in rollup, do you want to report it? The rollup website does not say anything about ignoring outpu.assetFileNames from transform().

@Anidetrix
Copy link
Owner

Anidetrix commented May 5, 2020 •
edited

No, that's not it. You can call emitFile with name and it will give you reference id right away, that's all fine. The problem being that if you try to use that id to get resulting asset filename using getFileName it will throw an error telling that filename is not available yet, which seems to be the intended behavior.

@dilyanpalauzov
Copy link
Author

My understanding is that, if assets are injected during the transform() hook with x = this.emitFile({type:'asset', name: '...'}), then calling this.getFileName(x) too early (before generateBundle() ) the system says that the filename is not available yet.

Why are the filenames of the assets needed before generateBundle()?

@Anidetrix
Copy link
Owner

Anidetrix commented May 6, 2020 •
edited

My understanding is that, if assets are injected during the transform() hook with x = this.emitFile({type:'asset', name: '...'}), then calling this.getFileName(x) too early (before generateBundle() ) the system says that the filename is not available yet.

Correct, although there are hooks before generateBundle which can get filename using reference id, they don't seem to be applicable to this situation.

Why are the filenames of the assets needed before generateBundle()?

First off, URLs are being modified right away, but it is theoretically possible to modify them during generateBundle using reference id as a placeholder, which would be set in transform hook instead of an actual URL, but it leads to another bigger problem - generateBundle approach would only work for "extract" mode, since in "inject" mode it would already be turned into chunk, tree-shaken and reorganized, and in "emit" mode it might turn into absolutely anything and theoretically might not even be in the build pipeline at this point.

@dilyanpalauzov
Copy link
Author

So during the transform() phase the url() assets need to be put on the right place without knowing the output.assetFileNames value. And putting the assets later on the right place can work for mode:extract, but not for modes “inject” or “emit”.

The description of "inject":

“(default) - Inject CSS into . You can also pass options for CSS injection. Alternatively, you can pass your own CSS injector.”

I would clarify further this description, stating that the CSS is embedded in the generated JS files and the JS files at runtime extend <head>.

Fair enough. Althougt output.assetFileNames could be taken implicilty into account when “extract” is used, this plugin has already a lot of functions and options with meaningful defaults.

@mnlbox mnlbox mentioned this issue Aug 12, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Anidetrix Anidetrix

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@dilyanpalauzov @Anidetrix