fix: bump @rollup/plugin-node-resolve to v13

[web-padawan]

What I did

1. Updated @rollup/plugin-node-resolve to v13 (again)

Fixes #1376
Fixes #1443
Fixes #1575
Fixes #1568

[changeset-bot]

🦋 Changeset detected

Latest commit: 617b1f3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@web/dev-server	Patch
@web/dev-server-rollup	Patch
rollup-plugin-workbox	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[bennypowers]

does the failing test mean users won't be able to use both plugins? 🤔

[web-padawan]

Here is one issue which could potentially affect other plugins: rollup/plugins#973

See also related code which returns false for external modules. This could be the problem.

[bennypowers]

Trying to figure out what's going on here, I noticed the test fails because it gets caught in an infinite loop where resolveId is called over and over with the same parameters:

resolveId {
  importee: '/Users/bennyp/Developer/web/packages/dev-server-rollup/test/node/fixtures/resolve-outside-dir/node_modules/module-a/index.js',
  importer: '/Users/bennyp/Developer/web/packages/dev-server-rollup/test/node/fixtures/resolve-outside-dir/src/app.js'
}

I'm debugging with

 ndb yarn mocha packages/dev-server-rollup/test/node/plugins/node-resolve.test.ts --require ts-node/register --watch --watch-extensions ts --watch-files 'packages/**/*.ts'     

• we get caught in a loop between the actual rollup plugin context and rollupAdapter's context
• debugger never reaches isOutsideRootDir from https://github.com/modernweb-dev/web/pull/1382/files
• debugger also never reaches the regexp test for __wds-outside-root__ from fix(dev-server): fixes #1536, correctly handle outside-root paths #1537

The loop looks like this:

1. the server calls rollupAdapter.ts' resolveImport for node-resolve with source module-a
2. rollupAdapter for node-resolve calls @rollup/plugin-node-resolve's resolveId with importee module-a
3. the ID resolves as ROOT/node_modules/module-a/index.js
4. the server calls rollupAdapter.ts' resolveImport for commonjs with ROOT/node_modules/module-a/index.js
5. rollupAdapter for commonjs calls @rollup/plugin-node-resolve's resolveId with importee ROOT/node_modules/module-a/index.js
6. server calls rollupAdapter.ts' resolveImport for node-resolve with ROOT/node_modules/module-a/index.js
7. rollupAdapter for node-resolve calls @rollup/plugin-node-resolve's resolveId with importee ROOT/node_modules/module-a/index.js
8. @rollup/plugin-node-resolve's resolveId resolves the ID as ROOT/node_modules/module-a/index.js
9. LOOP from step 4

Looking at that, do we need to mark the module as external in between step 3 and 4?

[bennypowers]

@tjenkinson would you mind taking a look if you have a moment? I suspect our loop is caused by the changes to rollup/plugin-node-resolve's use of rollup api (this.resolve), and the changes you made in rollup/plugins@3a543df

[tjenkinson]

So I haven’t looked in a lot of detail and don’t have much context on what this project is but from the docs it looks like it doesn’t use rollup but is mimicking the api?

If that’s the case you might need to implement the same changes that were made in rollup in rollup/rollup#4000 that change the behaviour of skipSelf.

With the old behaviour there would be an infinite loop in rollup so sounds like it could be that.

[bennypowers]

Thanks @tjenkinson that was very helpful.

@LarsDenBakker PTAL, thanks.

[LarsDenBakker]

@bennypowers your change will always skip over all plugins if skipSelf is true. We need to mimick what's being done inside rollup, where when one of the other plugins calls this.resolve again from its resolveId hook with the exact same source and importer, the initial plugin will be skipped as well.

Trying to look for the best way to do this, it might require some changes in core.

[Marshal27]

Ended up here due to not being able to import side-effects for selector registrations in the customElements registry... thank you all for the work on this one, I will be eagerly watching this in hopes that it lands soon.

[abdonrd]

Have you had time to see this? Thanks in advance!

[web-padawan]

I don't feel confident to dig into this 😞 And Lars is probably still away. Any help would be highly appreciated.

[gloriarodrife]

Any news? Thanks!

[Tansito]

I was following this PR and really thank you for the work! I assume there was no advances here, was it? 🤞

[daKmoR]

quite the investigation happened here... seems to be related to a change in the commonjs plugin 🤔

this "mixed" approach got superseded by #1942 which only updates the plugin-node-resolve

updating the dev dependencies plugin-commonjs and/or rollup may be done in a separate step? Is anyone up for it? (beware there may be dragons 😅 as with everything related to esm <-> cjs 🙈)