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

Add isEntry flag to resolveId and this.resolve #4230

Merged
merged 5 commits into from
Oct 1, 2021

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Sep 26, 2021

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

In the past, the way to detect if we are resolving an entry point in the resolveId hook was to check if the importer was undefined. However since we allowed to specify an importer in the this.emitFile hook, this check does not work in all cases any more.
This is problematic if you want to resolve entry points (and only those!) to proxies in a reliable manner.

This PR adds a new flag isEntry to the resolveId plugin hook to make this explicit. The same flag is also added to the this.resolve plugin context function. So resolving entries to plugins would now work like this:

function onlyDefaultForEntriesPlugin() {
  return {
    name: 'only-default-for-entries',
    async resolveId(source, importer, options) {
      if (options.isEntry) {
        // options contains `custom` and `isEntry`, so we are forwarding both
        const resolution = await this.resolve(source, importer, { skipSelf: true, ...options });
        if (!resolution) return null;
        return `${resolution.id}?entry-proxy`;
      }
      return null;
    },
    load(id) {
      if (id.endsWith('?entry-proxy')) {
        const importee = id.slice(0, -'?entry-proxy'.length);
        return `export {default} from '${importee}';`;
      }
      return null;
    }
  };
}

In the resolveId hook, the value of options.isEntry is determined like this:

  • If we are resolving an entry point or emitted chunk, it is true
  • If we are resolving a (dynamic or static) import, it is false
  • If we are resolving a call to this.resolve and isEntry is a boolean, this value is used, e.g. this.resolve('foo', 'bar', {isEntry: true})
  • If we are resolving a call to this.resolve and isEntry is undefined (or we did not specify options at all), it is true if importer is undefined, otherwise it is false

The reason I need this is that I want to do exactly that in a forthcoming PR for the commonjs plugin (with the goal of handling circular dependencies correctly).

Once this is release, plugins should check if they need to forward this flag when they use this.resolve. It should be possible to implement support for this flag in a manner that is compatible with older Rollup versions (here you would just forward undefined which would trigger the fallback logic).

@github-actions
Copy link

github-actions bot commented Sep 26, 2021

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#resolve-id-is-entry

or load it into the REPL:
https://rollupjs.org/repl/?pr=4230

@codecov
Copy link

codecov bot commented Sep 26, 2021

Codecov Report

Merging #4230 (e92d44a) into master (61dd792) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4230   +/-   ##
=======================================
  Coverage   98.39%   98.39%           
=======================================
  Files         203      203           
  Lines        7341     7342    +1     
  Branches     2087     2088    +1     
=======================================
+ Hits         7223     7224    +1     
  Misses         58       58           
  Partials       60       60           
Impacted Files Coverage Δ
src/utils/resolveId.ts 94.73% <ø> (ø)
src/ModuleLoader.ts 100.00% <100.00%> (ø)
src/utils/PluginContext.ts 100.00% <100.00%> (ø)
src/utils/resolveIdViaPlugins.ts 100.00% <100.00%> (ø)

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 61dd792...e92d44a. Read the comment docs.

@lukastaegert
Copy link
Member Author

@yyx990803 @patak-js tagging you as this is a (minor) extension to the plugin API that may or may not be relevant for you to implement. Please tell me if I should tag other people for such things in the future.

This flag will become relevant for the CommonJS plugin at some point (but there is a bigger one I am working on that will likely have more impact for you).

@guybedford
Copy link
Contributor

LGTM. Just to clarify here, a dynamic import that becomes an emitted chunk would have isEntry set to true right?

@patak-dev
Copy link
Contributor

Thanks for the heads up @lukastaegert. It is relevant, as we need to implement it in the plugin pipeline of Vite dev server.

LGTM. I started a branch in Vite to see what changes are needed on Vite side (see commit). We would need to review later with proper test cases but this looks straightforward.

About other people that may be interested in these changes, @antfu from Vite's side and @marvinhagemeister, as they would need to do the same for WMR

A note about extending the plugin API, Vite added an ssr boolean flag to resolveId, load, and transform. See docs. This doesn't interfere in resolveId as it goes after the options param. But in transform, there isn't an options object. @lukastaegert if you think we may need to pass an options object to transform in the future, maybe we are still on time to change this SSR extension on our side (passing an empty options for the moment as the third param, or passing the ssr inside an options object). The SSR API is still marked as experimental, so I think these changes are possible. Current extended hook for reference:

export function mySSRPlugin() {
  return {
    name: 'my-ssr',
    transform(code, id, ssr) {
      if (ssr) {
        // perform ssr-specific transform...
      }
    }
  }
}

@lukastaegert
Copy link
Member Author

@guybedford the dynamic import would be resolved twice: Once for the import itself where isEntry is false and once when it is emitted where it is set to true. This works well for the entry proxy use case: You would not want to add a proxy for an internal dynamic import, but you might want to add a proxy if it becomes part of the public API. If you need special handling for dynamic imports, implement the resolveDynamicImport hook.

@patak-js I will need to think about this more, but one first thought is that a plain boolean parameter may not be forward looking. If you want to add more parameters in the future, you will be happy if you make it an object now. At this point, one could consider merging with Rollup‘a object if you do not add too much stuff that Rollup cannot use in the future :)

@lukastaegert
Copy link
Member Author

lukastaegert commented Sep 27, 2021

@patak-js Expanding more on the idea of how to add a flag to Rollup's hooks that will not interfere with future extensions:

  • An object is generally a better idea than primitive positional parameters. Besides the fact that it may or may not collide with future extensions of the transform and load apis in Rollup, it can prove problematic for future Vite extensions as well. So e.g. imagine you add a "legacy" mode. If you add another positional parameter, then plugins that just need that parameter but not the ssr parameter still needs to provide the ssr parameter to get the other one. Also it is very hard to deprecate positional parameters as we learned in Rollup.
  • If we extend Rollup hooks, we try to go for option objects now, leaving the few positional parameters in place for legacy reasons.
  • Rollup does not and will not validate option objects for unknown fields. This is because otherwise, we would make it harder to write plugins that work well across different Rollup versions.
  • If you are afraid of potential future field name conflicts, you could consider namespacing vite parameters. These could be potential ways that I can guarantee will be future proof (and should not require you to clean up objects before passing them to Rollup):
    // Nested object
    interface Plugin {
      // ...
      resolveId?(
        source: string,
        importer: string | undefined,
        options: { custom?: CustomPluginOptions, vite?: {ssr?: boolean} }
      ): Promise<ResolveIdResult> | ResolveIdResult
    }
    
    // Field prefixes
    interface Plugin {
      // ...
      resolveId?(
        source: string,
        importer: string | undefined,
        options: { custom?: CustomPluginOptions, viteSsr?: boolean }
      ): Promise<ResolveIdResult> | ResolveIdResult
      load?(
        this: PluginContext,
        id: string,
        options: {viteSsr?: boolean}
      ): Promise<LoadResult> | LoadResult
      transform?(
        this: TransformPluginContext,
        code: string,
        id: string,
        options: {viteSsr?: boolean}
      ): Promise<TransformResult> | TransformResult
    }

@patak-dev
Copy link
Contributor

Thanks for taking the time to share your experience here @lukastaegert, moving this discussion to vitejs/vite#5109 to avoid derailing this PR.

@lukastaegert lukastaegert merged commit d0db534 into master Oct 1, 2021
@lukastaegert lukastaegert deleted the resolve-id-is-entry branch October 1, 2021 04:43
@marvinhagemeister
Copy link

@patak-js Thanks for tagging me!

Nothing more to add other than that this feature really helps us both vite and other bundlers 👍 So thanks @lukastaegert for adding it 🙌

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.

4 participants