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

test(run): add test broken in the latest rollup #241

Closed
wants to merge 1 commit into from

Conversation

TrySound
Copy link
Member

@TrySound TrySound commented Mar 4, 2020

#192 Rollup Plugin Name: run

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

I found case where new rollup fails with run plugin.
When entry point and dynamic import use the same module
chunk.modules becomes empty. Run checks chunk.modules[input] for some
reason which fails when chunk.modules is empty.

@lukastaegert could you take a look?

@TrySound TrySound requested a review from lukastaegert March 4, 2020 18:48
@TrySound
Copy link
Member Author

TrySound commented Mar 5, 2020

@lukastaegert I guess this happens because of circular dependencies. This is a tradeoff of reusing stuff in dynamic chunks. Would be good to have a warning for such cases. I didn't found any in my circular dependencies warnings though I could miss it because I have ~100 in graphql server.

@shellscape
Copy link
Collaborator

@TrySound a few CI issues to clear up there.

@TrySound TrySound changed the title test(run): add test broker in the latest rollup test(run): add test broken in the latest rollup Mar 28, 2020
I found case where new rollup fails with run plugin.
When entry point and dynamic import use the same module
chunk.modules becomes empty. Run checks `chunk.modules[input]` for some
reason which fails when chunk.modules is empty.
@lukastaegert
Copy link
Member

This does not always work and was already broken before Rollup 2: https://github.com/rollup/plugins/blob/master/packages/run/lib/index.js#L52-L55

The point is that when Rollup cannot match the export signature of an entry chunk because it needs to add additional exports, it will create a facade chunk that reexports exactly those exports that the original entry point exposed. A facade chunk will have its chunk.modules empty because it does not contain the code of any module, it just contains reexports.

However you can use the facadeModuleId to achieve what you want.

Also note that if a plugin just wants to read options, it should NOT use the options hook because other plugins can still mutate options at that point. If you just want to read, always use buildStart which receives the final options used by Rollup. Here is a proposal how you could rewrite the plugin to make it work:

const path = require('path');
const childProcess = require('child_process');

module.exports = (opts = {}) => {
  let input;
  let proc;

  const args = opts.args || [];
  const forkOptions = opts.options || opts;
  delete forkOptions.args;

  return {
    name: 'run',

    // As stated above, do not use options here
    buildStart(opts) {
      let inputs = opts.input;

      if (typeof inputs === 'string') {
        inputs = [inputs];
      }

      if (typeof inputs === 'object') {
        inputs = Object.values(inputs);
      }

      if (inputs.length > 1) {
        throw new Error(`@rollup/plugin-run only works with a single entry point`);
      }

      input = path.resolve(inputs[0]);
    },

    generateBundle(outputOptions, bundle, isWrite) {
      if (!isWrite) {
        this.error(`@rollup/plugin-run currently only works with bundles that are written to disk`);
      }
    },

    writeBundle(outputOptions, bundle) {
      const dir = outputOptions.dir || path.dirname(outputOptions.file);
      const entryFileName = Object.keys(bundle)
        .find(fileName => bundle[fileName].facadeModuleId === input);

      if (entryFileName) {
        if (proc) proc.kill();
        proc = childProcess.fork(path.join(dir, entryFileName), args, forkOptions);
      } else {
        this.error(`@rollup/plugin-run could not find output chunk`);
      }
    }
  };
};

@TrySound
Copy link
Member Author

Awesome! Thanks for response @lukastaegert. I'll try to cover all cases with tests.

TrySound added a commit that referenced this pull request Mar 31, 2020
Ref #241 (comment)

- take input from latest options when build is started
- check facade id instead of modules list
shellscape pushed a commit that referenced this pull request Apr 1, 2020
Ref #241 (comment)

- take input from latest options when build is started
- check facade id instead of modules list
shellscape pushed a commit that referenced this pull request Apr 1, 2020
…292)

Ref #241 (comment)

- take input from latest options when build is started
- check facade id instead of modules list
@TrySound TrySound closed this Apr 3, 2020
@TrySound TrySound deleted the run_empty_modules branch April 3, 2020 15:56
LarsDenBakker pushed a commit to LarsDenBakker/plugins that referenced this pull request Sep 12, 2020
…ollup#292)

Ref rollup#241 (comment)

- take input from latest options when build is started
- check facade id instead of modules list
vasttiankliu pushed a commit to vasttiankliu/plugins that referenced this pull request Nov 17, 2022
…(#292)

Ref rollup/plugins#241 (comment)

- take input from latest options when build is started
- check facade id instead of modules list
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants