search

Ephem / rollup-plugin-preserve-directives

A Rollup plugin to preserve directives like "use client" when preserveModules is true
MIT License
64 stars 8 forks source link

Does not support SCSS? #21

Closed simonerlandsen closed 7 months ago

simonerlandsen commented 7 months ago

I have react components that imports scss files. When adding this plugin I get this error on build:

[preserve-directives] Expression expected
file: --redacted--/packages/buflib/src/components/responsive_image/ResponsiveImage.scss
error during build:
RollupError: Expression expected
    at error (file://--redacted--/node_modules/vite/node_modules/rollup/dist/es/shared/parseAst.js:337:30)
    at parseError (file://--redacted--/node_modules/vite/node_modules/rollup/dist/es/shared/parseAst.js:972:9)
    at convertNode (file://--redacted--/node_modules/vite/node_modules/rollup/dist/es/shared/parseAst.js:2057:12)
    at convertProgram (file://--redacted--/node_modules/vite/node_modules/rollup/dist/es/shared/parseAst.js:965:12)
    at Object.parseAst [as parse] (file://--redacted--/node_modules/vite/node_modules/rollup/dist/es/shared/parseAst.js:2104:12)
    at Object.transform (file://--redacted--/node_modules/rollup-plugin-preserve-directives/dist/index.js:12:26)
    at file://--redacted--/node_modules/vite/node_modules/rollup/dist/es/shared/node-entry.js:18692:40

versions:

"vite-plugin-dts": "^3.7.2",
"vite": "^5.0.12"
Ephem commented 7 months ago

Hey! Thanks for reporting this.

We did just add an exception to not parse .css files: https://github.com/Ephem/rollup-plugin-preserve-directives/pull/20

I'm not super well versed in Rollup actually so I'm not quite sure how this is supposed to work in a scalable fashion though. I'd be happy to add .scss-files to that list too, but I have a feeling that's not the correct approach long term, there are a bunch of possible files that could be imported after all and going by file extension also feels brittle. Catching the parsing error and ignoring it could be another way I guess..? If it's a "real" parsing error I guess that would bubble up elsewhere, maybe?

I don't have that much time to research this at the moment, so if anyone knows how Rollup plugins usually handle this I would love to hear! I would happily accept a PR to add .scss to the existing .css filter too just to unblock that case specifically.

simonerlandsen commented 7 months ago

@Ephem Thanks alot for your response, and not least for tackling the core issue that this plugin is addressing, for anyone who needs it.

I am not versed in developing rollup plugins, so I do not know how scss usually is dealt with, but I will see about adding scss to the existing filter in a PR on Monday.

Again, thank you for the response 🙏

adbutterfield commented 7 months ago

I just hit the same issue, but with .pcss files.

simonerlandsen commented 7 months ago

@adbutterfield I am working on a PR that will let us add file types to ignore by passing an array in an option. Will add the PR soon.

Ephem commented 7 months ago

@adbutterfield Thanks for reporting that!

@simonerlandsen That sounds like a great pragmatic approach, looking forward to it!

simonerlandsen commented 7 months ago

@Ephem I don't want to nag, but do you have any idea of when you will be able to look at the PR (and hopefully publish a new versjon)?

Ephem commented 7 months ago

No worries! Busy week but managed to find the time this morning so it's now been released as v0.4.0, thanks again for posting the issue and taking the time to submit a fix! ❤️

simonerlandsen commented 7 months ago

Lovely! I've tested 0.4.0 and it's working for me. @adbutterfield this should hopefully solve your issue as well, by you adding an exclude pattern in the options like so:

preserveDirectives({ exclude: ['**/*.pcss'] })