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

Rework svelte & clean up rollup integrations #430

Merged
merged 14 commits into from Jun 8, 2018

Conversation

Projects
None yet
2 participants
@tivac
Copy link
Owner

commented Jun 8, 2018

  • Svelte no longer has a custom rollup integration, use modular-css-rollup instead
  • modular-css-rollup now supports a common option that will handle outputting any CSS that was removed from chunks due to treeshaking.
  • modular-css-rollup accepts a new processor option that is expected to be a fully-configured & instantiated instance of Processor from modular-css-core.
  • Updated svelte docs

tivac added some commits Jun 7, 2018

feat: add common config value
It's sort of a hack but works around rollup tree-shaking away the CSS imports (since they aren't technically being used by the JS code).
fix: remove svelte rollup integration
It's very simple and bad!
feat: rework svelte markup handling
Also improve test coverage!
fix: inject relative path
Absolutes are a) silly and b) impossible to snapshot
@guybedford

This comment has been minimized.

Copy link

commented on packages/rollup/rollup.js in 451d802 Jun 8, 2018

Do you not need the "to" here to be the asset file path instead of its id?

This comment has been minimized.

Copy link
Owner Author

replied Jun 8, 2018

Oof, yes thanks. Not sure how I'll resolve that atm but I'll think on it.

This comment has been minimized.

Copy link

replied Jun 10, 2018

This should just be to: this.getAssetFileName(css) I think, possibly even path.resolve(outputOptions.dir, this.getAssetFileName(css)).

@guybedford

This comment has been minimized.

Copy link

commented on packages/rollup/rollup.js in 451d802 Jun 8, 2018

Yes it should be possible to just have const css = this.emitAsset(``${base}.css``);, as long as there is the this.setAssetSource call below you are fine.

This comment has been minimized.

Copy link
Owner Author

replied Jun 8, 2018

I get an error.

Plugin error creating asset watch-output.css - no asset source set.

      145 |                     // TODO: docs say that empty string arg to .emitAsset() shouldn't be required
      146 |                     // https://github.com/rollup/rollup/wiki/Plugins#plugin-context
    > 147 |                     const css = this.emitAsset(`${base}.css`);
          |                                      ^
      148 |
      149 |                     const result = await processor.output({
      150 |                         from : source,

      at error (node_modules/rollup/dist/rollup.js:183:15)
      at getAssetFileName (node_modules/rollup/dist/rollup.js:20480:9)
      at finaliseAsset (node_modules/rollup/dist/rollup.js:20551:20)
      at Object.emitAsset (node_modules/rollup/dist/rollup.js:20514:17)
      at Promise.all.bundles.map (packages/rollup/rollup.js:147:38)
          at Array.map (<anonymous>)
      at Object.generateBundle (packages/rollup/rollup.js:144:25)
      at node_modules/rollup/dist/rollup.js:21690:54
          at Array.map (<anonymous>)
      at node_modules/rollup/dist/rollup.js:21689:62

This comment has been minimized.

Copy link

replied Jun 10, 2018

Thanks, just posted a fix at rollup/rollup#2246.

@guybedford

This comment has been minimized.

Copy link

commented on packages/rollup/rollup.js in 451d802 Jun 8, 2018

Is this the filter hack you are referring to?

This comment has been minimized.

Copy link
Owner Author

replied Jun 8, 2018

Yeah, all the CSS dependencies inject import from "foo.css"; lines but they get shaken out of the rollup-created bundles. This "common bundle" thing is my somewhat hacky way of figuring out what didn't get put into a rollup-created bundle and making sure it doesn't get lost.

This comment has been minimized.

Copy link

replied Jun 10, 2018

As mentioned, I believe handling this in transform dependencies sounds like a suitable fix then?

tivac added some commits Jun 8, 2018

fix: relative paths in rollup
Also remove some very repetitive paths
@codecov

This comment has been minimized.

Copy link

commented Jun 8, 2018

Codecov Report

Merging #430 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #430      +/-   ##
==========================================
- Coverage    99.1%   99.08%   -0.02%     
==========================================
  Files          36       31       -5     
  Lines         779      765      -14     
  Branches      113      114       +1     
==========================================
- Hits          772      758      -14     
  Misses          7        7
Impacted Files Coverage Δ
packages/svelte/svelte.js 100% <100%> (ø) ⬆️
packages/svelte/src/markup.js 100% <100%> (ø)
packages/rollup/rollup.js 100% <100%> (ø) ⬆️
packages/svelte/src/style.js 100% <100%> (ø)
packages/test-utils/read.js
packages/test-utils/namer.js
packages/test-utils/exists.js
packages/test-utils/prefix.js
packages/test-utils/relative.js
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6746694...3009910. Read the comment docs.

@tivac tivac merged commit c80dafe into master Jun 8, 2018

4 checks passed

codecov/patch 100% of diff hit (target 99.1%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +0.89% compared to 6746694
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tivac tivac deleted the rollup-imports branch Jun 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.