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

fix: use this.emitFile() API from rollup #666

Merged
merged 4 commits into from
Sep 15, 2019
Merged

fix: use this.emitFile() API from rollup #666

merged 4 commits into from
Sep 15, 2019

Conversation

tivac
Copy link
Owner

@tivac tivac commented Sep 11, 2019

Description

Using this.emitFile() for all file emission from the rollup plugin.

Motivation and Context

this.emitFile() is the correct future-proof way to emit files from rollup plugins. this.emitAsset() and this.emitChunk() are being deprecated, and it was never really supported to add files directly to the bundle the way they were.

How Has This Been Tested?

Normal test suite

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@tivac tivac self-assigned this Sep 11, 2019
@TravisBuddy
Copy link

Travis tests have failed

Hey @tivac,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

Node.js: 8

View build log

npm test -- --ci
> modular-css@0.0.0 pretest /home/travis/build/tivac/modular-css
> npm run parsers


> modular-css@0.0.0 parsers /home/travis/build/tivac/modular-css
> node parsers/build.js


> modular-css@0.0.0 test /home/travis/build/tivac/modular-css
> jest "--ci"

PASS tests packages/rollup/test/rollup.test.js
PASS tests packages/processor/test/options.test.js
PASS tests packages/processor/test/api.test.js
PASS tests packages/rollup/test/watch.test.js
PASS tests packages/rollup/test/splitting.test.js
PASS tests packages/svelte/test/svelte.test.js
(node:5516) DeprecationWarning: Tapable.plugin is deprecated. Use new API on `.hooks` instead
PASS tests packages/webpack/test/webpack.test.js
FAIL tests packages/svelte/test/rollup.test.js
  ● /svelte.js › rollup watching › should generate updated output when composition changes

    expect(received).toMatchSnapshot()

    Snapshot name: `/svelte.js rollup watching should generate updated output when composition changes 1`

    - Snapshot
    + Received

      Snapshot Diff:
    - - First value
    - + Second value
    - 
    - @@ -173,11 +173,11 @@
    -   
    -   	return {
    -   		c() {
    -   			div = createElement("div");
    -   			div.textContent = "Hi";
    - - 			div.className = "mc8c3b9f40_b mc7c0d2c82_a";
    - + 			div.className = "mc8c3b9f40_c mc7c0d2c82_a";
    -   		},
    -   
    -   		m(target, anchor) {
    -   			insert(target, div, anchor);
    -   		},
    + Compared values have no visual difference.

      172 |                 v2 = dir("./rollup-composes/output/");
      173 | 
    > 174 |                 expect(v1).toMatchDiffSnapshot(v2);
          |                            ^
      175 | 
      176 |                 return done();
      177 |             }));

      at toMatchDiffSnapshot (packages/svelte/test/rollup.test.js:174:28)
      at Object.cb (packages/test-utils/rollup-watching.js:17:9)
      at Watcher.emit (node_modules/rollup/dist/rollup.js:20770:22)
      at taskPromise.then (node_modules/rollup/dist/rollup.js:20803:18)

 › 1 snapshot failed.
PASS tests packages/browserify/test/factor-bundle.test.js
PASS tests packages/browserify/test/browserify.test.js
PASS tests packages/processor/test/composition.test.js
PASS tests packages/rollup-rewriter/test/rewriter.test.js
PASS tests packages/processor/test/values.test.js
PASS tests packages/postcss/test/postcss.test.js
PASS tests packages/browserify/test/watchify.test.js
PASS tests packages/processor/test/at-composes.test.js
PASS tests packages/aliases/test/aliases.test.js
PASS tests packages/processor/test/scoping.test.js
PASS tests packages/processor/test/keyframes.test.js
PASS tests packages/browserify/test/issue-58.test.js
PASS tests packages/browserify/test/issue-313.test.js
PASS tests packages/cli/test/cli.test.js
PASS tests packages/paths/test/paths.test.js
PASS tests packages/processor/test/getters.test.js
PASS tests packages/processor/test/externals.test.js
PASS tests packages/namer/test/namer.test.js
PASS tests packages/glob/test/glob.test.js
PASS tests packages/processor/test/exports.test.js
PASS tests packages/processor/test/issues/issue-56.test.js
PASS tests packages/processor/test/issues/issue-24.test.js
PASS tests packages/processor/test/issues/issue-98.test.js
PASS tests packages/processor/test/unicode.test.js
PASS tests packages/processor/test/issues/issue-66.test.js
PASS tests packages/processor/test/issues/issue-261.test.js

Summary of all failing tests
FAIL packages/svelte/test/rollup.test.js
  ● /svelte.js › rollup watching › should generate updated output when composition changes

    expect(received).toMatchSnapshot()

    Snapshot name: `/svelte.js rollup watching should generate updated output when composition changes 1`

    - Snapshot
    + Received

      Snapshot Diff:
    - - First value
    - + Second value
    - 
    - @@ -173,11 +173,11 @@
    -   
    -   	return {
    -   		c() {
    -   			div = createElement("div");
    -   			div.textContent = "Hi";
    - - 			div.className = "mc8c3b9f40_b mc7c0d2c82_a";
    - + 			div.className = "mc8c3b9f40_c mc7c0d2c82_a";
    -   		},
    -   
    -   		m(target, anchor) {
    -   			insert(target, div, anchor);
    -   		},
    + Compared values have no visual difference.

      172 |                 v2 = dir("./rollup-composes/output/");
      173 | 
    > 174 |                 expect(v1).toMatchDiffSnapshot(v2);
          |                            ^
      175 | 
      176 |                 return done();
      177 |             }));

      at toMatchDiffSnapshot (packages/svelte/test/rollup.test.js:174:28)
      at Object.cb (packages/test-utils/rollup-watching.js:17:9)
      at Watcher.emit (node_modules/rollup/dist/rollup.js:20770:22)
      at taskPromise.then (node_modules/rollup/dist/rollup.js:20803:18)


Snapshot Summary
 › 1 snapshot failed from 1 test suite. Inspect your code changes or run `npm test -- -u` to update them.

Test Suites: 1 failed, 1 skipped, 33 passed, 34 of 35 total
Tests:       1 failed, 5 skipped, 282 passed, 288 total
Snapshots:   1 failed, 318 passed, 319 total
Time:        23.597s
Ran all test suites.
npm ERR! Test failed.  See above for more details.
TravisBuddy Request Identifier: 827b1a20-d462-11e9-847a-07722ef8bbdd

@tivac
Copy link
Owner Author

tivac commented Sep 11, 2019

FAIL tests packages/svelte/test/rollup.test.js
  ● /svelte.js › rollup watching › should generate updated output when composition changes

    expect(received).toMatchSnapshot()

    Snapshot name: `/svelte.js rollup watching should generate updated output when composition changes 1`

    - Snapshot
    + Received

      Snapshot Diff:
    - - First value
    - + Second value
    - 
    - @@ -173,11 +173,11 @@
    -   
    -   	return {
    -   		c() {
    -   			div = createElement("div");
    -   			div.textContent = "Hi";
    - - 			div.className = "mc8c3b9f40_b mc7c0d2c82_a";
    - + 			div.className = "mc8c3b9f40_c mc7c0d2c82_a";
    -   		},
    -   
    -   		m(target, anchor) {
    -   			insert(target, div, anchor);
    -   		},
    + Compared values have no visual difference.

      172 |                 v2 = dir("./rollup-composes/output/");
      173 | 
    > 174 |                 expect(v1).toMatchDiffSnapshot(v2);
          |                            ^
      175 | 
      176 |                 return done();
      177 |             }));

      at toMatchDiffSnapshot (packages/svelte/test/rollup.test.js:174:28)
      at Object.cb (packages/test-utils/rollup-watching.js:17:9)
      at Watcher.emit (node_modules/rollup/dist/rollup.js:20770:22)
      at taskPromise.then (node_modules/rollup/dist/rollup.js:20803:18)
- First value
+ Second value

@@ -173,11 +173,11 @@
  
  	return {
  		c() {
  			div = createElement("div");
  			div.textContent = "Hi";
- 			div.className = "mc8c3b9f40_b mc7c0d2c82_a";
+ 			div.className = "mc8c3b9f40_c mc7c0d2c82_a";
  		},
  
  		m(target, anchor) {
  			insert(target, div, anchor);
  		},

That test claiming to have no diff now is very suspicious & broken, that line should definitely be changing after the composes reference changes.

@tivac
Copy link
Owner Author

tivac commented Sep 12, 2019

This broke in between rollup@1.20.3 and rollup@1.21.0. Thinking about it more I'm kinda surprised it ever worked though? 🤔

Rollup seems like it's doing the right thing here, which is only transforming the .css file that changed. There's no reason it would re-transform the .html, that file didn't change on disk. Since it doesn't re-run the transform hook for that file it doesn't run the preprocessor either and we've now got updated CSS but the svelte component is out of date.

Crap.

@tivac
Copy link
Owner Author

tivac commented Sep 12, 2019

Found the specific change that broke this:

rollup/rollup@v1.20.3...v1.21.0#diff-093a6241a0003efb710688c0b3332f53L183-R186

invalidate(id: string, isTransformDependency: boolean) {
    this.invalidated = true;
    if (isTransformDependency) {
-        (this.cache.modules as ModuleJSON[]).forEach(module => {
-            if (!module.transformDependencies || module.transformDependencies.indexOf(id) === -1)
-                return;
+        for (const module of this.cache.modules) {
+            if (module.transformDependencies.indexOf(id) === -1) return;
            // effective invalidation
            module.originalCode = null as any;
-        });
+        }
    }
    this.watcher.invalidate(id);
}

When this code was changed from this.cache.modules.forEach(..) to for(const module of this.cache.modules) { ... }, the return wasn't changed to a continue, so as soon as the first module that doesn't have a matching transformDependency is found it halts iteration.

I need to report this to rollup (& maybe send a PR since the fix is trivial).

@tivac
Copy link
Owner Author

tivac commented Sep 12, 2019

Bug: rollup/rollup#3111
PR: rollup/rollup#3112

Which fixes the weird rebuild issue I found.
@codecov
Copy link

codecov bot commented Sep 15, 2019

Codecov Report

Merging #666 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #666      +/-   ##
==========================================
+ Coverage   99.12%   99.12%   +<.01%     
==========================================
  Files          45       45              
  Lines        1138     1149      +11     
  Branches      173      177       +4     
==========================================
+ Hits         1128     1139      +11     
  Misses         10       10
Impacted Files Coverage Δ
packages/rollup/rollup.js 100% <100%> (ø) ⬆️
packages/rollup-rewriter/rewriter.js 100% <0%> (ø) ⬆️

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 6d81d3f...9b4da87. Read the comment docs.

@tivac tivac merged commit 108a4a1 into master Sep 15, 2019
@tivac tivac deleted the rollup-emit-file branch September 15, 2019 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants