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

`@value` are included in bundle when using the rollup plugin #672

Closed
lunsdorf opened this issue Sep 20, 2019 · 2 comments

Comments

@lunsdorf
Copy link
Contributor

commented Sep 20, 2019

Expected Behavior

Values, which are imported via @value myValue from "./foo.css" are not bundled into the final JS files when using the rollup plugin.

Current Behavior

Values do not show up in bundled JS files.

/* === colors.css === */
@value main: red;
@value bg: white;

/* === site.css === */
@value main from "./colors.css";

body {
    color: main;
}
import css from "./site.css"

// css is:
/*
{
    body: "dafdfcc_body"
    main: "red"
}
*/

Possible Solution

This happens inside the _add function in the @modular-css/processor package. I'm not sure why the values are included, but I deleted these lines in my local setup and couldn't find any issues. Is it possible to remove them from the export in general? Or maybe, if this is actually needed by other packages, could this be set as config option so that the rollup plugin could disable it?

Steps to Reproduce (for bugs)

  1. setup rollup with @modular-css/rollup
  2. create a CSS file with @values
  3. create another CSS file that imports these values
  4. import the second CSS file in your script using the default export
  5. invoke rollup to bundle the files
  6. find the @values definition inside the bundled code

Context

Having many @value imports pollutes the final bundle with unused code.

Your Environment

Executable Version
@modular-css/rollup 24.2.2
npm --version 6.4.1
node --version 10.15.3
OS Version
NAME VERSION
macOS Sierra 10.12.3

PS: This package is awesome!

@tivac

This comment has been minimized.

Copy link
Owner

commented Sep 20, 2019

Not an accident, see #396 where this behavior was requested and #398 where it was implemented.

I'm open to it being an option (that defaults to true to avoid a breaking change) and would accept a PR to support that. For consistency across all the implementations the changes should happen in the processor package.

@lunsdorf lunsdorf referenced this issue Sep 21, 2019
7 of 9 tasks complete
@lunsdorf

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2019

Okay, I created a PR that adds a config option exportValues to the processor which defaults to true. I realized the rollup-plugin as options named namedExports and styleExport, but I decided to stick to the naming used by the processor.

I had an issue running the tests on my local machine: for some reason the test for /rollup.js › watch mode › should generate updated output failed because the the file packages/rollup/test/output/watch/change/watched.js wasn't created. I'm not sure why or if this relates to my changes.

@tivac tivac closed this in 97a5b1e Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.