fix(node-resolve): do not ignore exceptions #564

tjenkinson · 2020-09-05T19:09:05Z

Rollup Plugin Name: `node-resolve`

This PR contains:

Are tests included?

yes kind of
no

Breaking Changes?

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

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

Description

recommend viewing with indentation disabled on github

I was looking to add a feature to return false from resolveId for builtins when preferBuiltins was enabled but noticed code wasn't running that I was expecting. I will follow up with this in a different PR (which I'll rebase that when this PR is finished).

There was a try/catch which was ignoring any exception and returning null from resolveId. I tried removing that block and it caused test failures, and was why my changes weren't working 😱.

I fixed the hidden issues to make the tests pass again. The 'root-dir' test looked like it was a bug in the test itself so I updated that test.

I think it would be better to let any unexpected errors throw out from resolveId and fail a build instead of silently converting them to a null return. If you'd like that back maybe we can figure out a way to have that logic disabled for tests.

as not being able to resolve the id is a valid output

this was passing before because the real error was being ignored

tjenkinson · 2020-09-05T19:12:10Z

packages/node-resolve/src/index.js

+      if (/\0/.test(importer)) {
+        // handle cases like common-js plugin which has form
+        // \u0000<id>?commonjs-proxy
+        importer = importer.slice(1);


not sure if there's a better way to handle this case?

If you remove this change you'll see the pkg.browser with mapping to prevent bundle by specifying a value of false test fail because the call to resolveId with the null byte is not allowed

That one is a really good question that got overlooked so far. I do not agree with the solution, though. Files starting with \0 are under the sole maintenance of some plugin, and we should not make assumptions about what the part behind the \0 means.
I think a valid approach might be to treat the importer like undefined, which is the value that gets passed for entry points. At least for the commonjs plugin, this should work nicely as all imports from those proxy files are fully resolved absolute paths anyway, and I would expect the same from other plugins as well.

I think a valid approach might be to treat the importer like undefined, which is the value that gets passed for entry points

@lukastaegert I pushed a commit to do this

tjenkinson · 2020-09-05T19:40:49Z

packages/node-resolve/test/root-dir.js

 test('deduplicates modules from the given root directory', async (t) => {
  const bundle = await rollup({
-    input: 'index.js',
+    input: './packages/package-a/index.js',


this was causing an error because the input could not be found. It should be relative to the rootDir right?

tjenkinson · 2020-10-11T13:01:42Z

bump :)

danielgindi · 2020-10-11T13:08:39Z

I'm a bit confused. I didn't see anywhere that you presented a specific use case, or your specific frontal-accident with this plugin...

Was it trying to somehow resolve modules that are actually internal commonjs proxies? Or was it just the importer being a commonjs proxy that broke things?

And are you now throwing the errors, failing the whole pipeline - or emitting warnings? (Because I see you removed the original try-catch, instead of altering it)

tjenkinson · 2020-10-11T16:15:11Z

Hi @danielgindi

I didn't see anywhere that you presented a specific use case, or your specific frontal-accident with this plugin...

The thing I was trying to do which caused me to stumble into this was the linked PR. I was looking to make builtin modules be treated as external.

Was it trying to somehow resolve modules that are actually internal commonjs proxies?

Maybe I should have been clearer in the description, but the exact cause of the problem was that a builtin module id would currently cause realpathSync to throw, and that error would bubble all the way up to the catch which then returned null.

And are you now throwing the errors, failing the whole pipeline - or emitting warnings?

There isn't the global handler anymore that converts errors to null. IMO catch should be used to handle specific errors and not as a global catch all. If there's an unexpected error we're not expecting to handle I think letting it bubble up to rollup and probably fail the build is the best outcome. Otherwise it silently catches things and issues can go unnoticed, like the deduplicates modules from the given root directory test for example which was actually broken.

danielgindi · 2020-10-11T17:05:42Z

@tjenkinson so basically avoid having to specify extra externals in the rollup level?

About the exceptions- I'm not sure I agree.
In general what you are saying is true.
But there are cases where a warning is more appropriate. If you failed to resolve a module (for whatever unknown reason), you don't necessarily want to stop everything.
Other modules print a warning, suggesting a possible root cause.
This means that if there is a failure in node-resolve, the user in some cases might be able to do nothing to bypass the error if he deems the error "ah, that's fine by me".

Also- removing the catch is a BREAKING CHANGE in that manner, and you did not mark it as one in the intro ;)

tjenkinson · 2020-10-11T17:29:15Z

so basically avoid having to specify extra externals in the rollup level?

yep this was the idea (but I decided to keep that separate to this PR)

But there are cases where a warning is more appropriate. If you failed to resolve a module (for whatever unknown reason), you don't necessarily want to stop everything.

Happy to put the catch back and log a warning in there if people would prefer that. However IMO as a developer I'd want there to be a blocker if something unexpected went wrong. Especially when some of the options like jail seem like they're meant to to prevent things outside a certain location being imported? I'd prefer it to fail safe and stop on an issue then return null and hand off to the next resolver.

Also- removing the catch is a BREAKING CHANGE in that manner, and you did not mark it as one in the intro ;)

Yeh I wasn't sure on this. I'm fine with it being breaking (or putting the catch back but with at least a warning log :) ). Also disabling that catch for tests would be nice.

danielgindi · 2020-10-11T18:41:33Z

Happy to put the catch back and log a warning in there if people would prefer that. However IMO as a developer I'd want there to be a blocker if something unexpected went wrong. Especially when some of the options like jail seem like they're meant to to prevent things outside a certain location being imported? I'd prefer it to fail safe and stop on an issue then return null and hand off to the next resolver.

I guess we need more opinions here

Yeh I wasn't sure on this. I'm fine with it being breaking (or putting the catch back but with at least a warning log :) ). Also disabling that catch for tests would be nice.

Btw, if only addings warnings to rollup, you could look at other test (i.e in commonjs) - some of them test for the existing of a warning.
So if it's the expected flow, to have a warning - or even an exception - then you should be expecting that in a test and make sure it happens.

LarsDenBakker · 2020-10-11T18:48:40Z

In my opinion a failure to resolve a module should result in an error and not into making the module an external, that's something that confuses users a lot currently. However this is not something that should be handled by the node resolve plugin, it should be on the level of rollup. There could be multiple plugins who resolve an id, and node-resolve shouldn't block further resolving.

tjenkinson · 2020-10-11T18:57:23Z

In my opinion a failure to resolve a module should result in an error and not into making the module an external, that's something that confuses users a lot currently.

AFAIK this becomes a warning from rollup if all resolveId hooks have returned null. Do you mean you'd prefer rollup to error there instead of warn? I think I agree, but not sure how it relates to this PR?

Also by "failure to resolve a module" do you mean a controlled failure such as the module not existing, or both controlled and uncontrolled failures like the file system throwing an error?

There could be multiple plugins who resolve an id, and node-resolve shouldn't block further resolving.

Also agree. The question is if an unexpected error occurs internally to the plugin, should we propagate this up to rollup, or convert this to null like we do now to hand over to the other resolvers (and log a warning)?

LarsDenBakker · 2020-10-11T19:01:13Z

An unexpected error should result in a failure, but I thought the try/catch catches errors thrown by the resolve package when a module couldn't by found. That's how it relates to this PR.

tjenkinson · 2020-10-11T19:54:30Z

but I thought the try/catch catches errors thrown by the resolve package when a module couldn't by found

Yep it looks that way, but IMO it's clearer to handle a module not being found in the main code path as an expected case (which is what this PR does), not the exception path.

When I started my PR to support automatically making builtins external, I was surprised when my code wasn't running because we were silently ending up in that catch block as the result of trying to read fs for example throwing given it's not a real file.

I guess I could take a different approach, and put the logic in the catch block to see if it's a builtin module, but that doesn't feel nice.

lukastaegert · 2020-10-13T04:05:44Z

I agree that this big catch was really bad for testing, but the general idea of course was that when this plugin cannot resolve a module, other resolvers should get a chance to do so before throwing.

So the question to me is that is not entirely clear to me from looking at the code: Will this still be the case for all "relevant" cases? And by that I mean it is definitely ok to throw if there is a weird file system error or some error that results from data in an unexpected format being passed to this plugin.

If you think this is the case, we could try to release this fix but be prepared to do a quick rollback in case we missed something important here.

In my opinion a failure to resolve a module should result in an error and not into making the module an external, that's something that confuses users a lot currently. However this is not something that should be handled by the node resolve plugin, it should be on the level of rollup. There could be multiple plugins who resolve an id, and node-resolve shouldn't block further resolving.

@LarsDenBakker If you say so, we can talk about this, could be a change in the next major version. Personally, I like this behaviour for quick prototyping, but if it confuses newcomers, then that is bad. Rollup could augment the error with instructions on how to mark the specific import as external to solve the issue.

I also think a lot of the woes that make this feature helpful is that builtins are not marked as external when using the preferBuiltins option in this plugin. As I understand @tjenkinson is working on a fix for just that, then one more argument for making this an error in Rollup.

tjenkinson · 2020-10-13T19:47:48Z

Will this still be the case for all "relevant" cases? And by that I mean it is definitely ok to throw if there is a weird file system error or some error that results from data in an unexpected format being passed to this plugin.

From my understanding yes it will. I've gone through everything that was in the try block looking at what could throw and nothing jumps out.

The one I'd like confirmation on is

      if (options.modulesOnly) {
        const code = await readFile(resolved, 'utf-8');
        if (isModule(code)) {
          return {
            id: `${resolved}${importSuffix}`,
            moduleSideEffects: hasModuleSideEffects(resolved)
          };
        }
        return null;
      }

Do we need to check a file at resolved exists here?

I also think a lot of the woes that make this feature helpful is that builtins are not marked as external when using the preferBuiltins option in this plugin. As I understand @tjenkinson is working on a fix for just that, then one more argument for making this an error in Rollup.

Yeh that's what I started here but thought I'd defer that until after this one.

like we do in index.js

…eptions

lukastaegert · 2020-10-14T03:43:46Z

Do we need to check a file at resolved exists here?

Looking at what this option is supposed to be doing and the current implementation, I am leaning towards file-not-found errors should also return null instead of throwing here (while other errors should throw), but that is an opinion.

tjenkinson · 2020-10-14T19:58:12Z

Done.

Wondering if

      if (/\0/.test(importer)) {
        importer = undefined;
      }

is a breaking change?

lukastaegert · 2020-10-15T04:46:54Z

Hard to say. I am not aware of any official plugins for which this would be breaking, but it is possible for third party plugins.

tjenkinson · 2020-10-24T10:17:18Z

Soooooo. I think this PR is ready to go? Don't see any other suggestions/concerns atm.

shellscape · 2020-10-25T13:37:48Z

tjenkinson · 2020-11-01T10:33:03Z

Here is the next PR to make builtins external :)

#627

tjenkinson added 7 commits September 5, 2020 19:50

do not ignore errors

39c9ce1

do not error when import specifier does not map to file

b456520

always swallow MODULE_NOT_FOUND errors

d78fa0f

as not being able to resolve the id is a valid output

return null from resolveId early if can't resolve

81890b7

strip null byte from importer if there is one

edffa14

fix root-dir test

b4daacd

this was passing before because the real error was being ignored

remove try/catch

b321a7d

tjenkinson commented Sep 5, 2020

View reviewed changes

tjenkinson marked this pull request as ready for review September 5, 2020 19:12

tjenkinson mentioned this pull request Sep 5, 2020

feat(node-resolve) Mark built-ins as external tjenkinson/plugins#1

Closed

9 tasks

tjenkinson commented Sep 5, 2020

View reviewed changes

shellscape requested review from LarsDenBakker, TrySound, Andarist, danielgindi, guybedford and lukastaegert September 6, 2020 16:26

shellscape added pr⁴ 🔧 fix 🔌 plugin-node-resolve labels Sep 6, 2020

tjenkinson added 3 commits October 13, 2020 20:52

use async exists() and realpath()

44227e4

like we do in index.js

Merge remote-tracking branch 'upstream/master' into do-not-ignore-exc…

3c7b64a

…eptions

pretend importer is undefined if it starts will \0

26f9bd2

return null if file not found with modulesOnly option

994ab6b

NotWoods force-pushed the master branch from e747f6b to 74d5dab Compare October 22, 2020 16:37

lukastaegert approved these changes Oct 25, 2020

View reviewed changes

danielgindi approved these changes Oct 25, 2020

View reviewed changes

shellscape merged commit 1459cf0 into rollup:master Oct 25, 2020

tjenkinson mentioned this pull request Nov 1, 2020

feat(node-resolve): Mark built-ins as external #627

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(node-resolve): do not ignore exceptions #564

fix(node-resolve): do not ignore exceptions #564

tjenkinson commented Sep 5, 2020 •

edited

tjenkinson Sep 5, 2020

lukastaegert Oct 13, 2020

tjenkinson Oct 13, 2020

tjenkinson Sep 5, 2020

tjenkinson commented Oct 11, 2020

danielgindi commented Oct 11, 2020

tjenkinson commented Oct 11, 2020

danielgindi commented Oct 11, 2020

tjenkinson commented Oct 11, 2020

danielgindi commented Oct 11, 2020

LarsDenBakker commented Oct 11, 2020

tjenkinson commented Oct 11, 2020

LarsDenBakker commented Oct 11, 2020

tjenkinson commented Oct 11, 2020 •

edited

lukastaegert commented Oct 13, 2020

tjenkinson commented Oct 13, 2020

lukastaegert commented Oct 14, 2020

tjenkinson commented Oct 14, 2020

lukastaegert commented Oct 15, 2020

tjenkinson commented Oct 24, 2020

shellscape commented Oct 25, 2020

tjenkinson commented Nov 1, 2020

fix(node-resolve): do not ignore exceptions #564

fix(node-resolve): do not ignore exceptions #564

Conversation

tjenkinson commented Sep 5, 2020 • edited

Rollup Plugin Name: node-resolve

Description

tjenkinson Sep 5, 2020

Choose a reason for hiding this comment

lukastaegert Oct 13, 2020

Choose a reason for hiding this comment

tjenkinson Oct 13, 2020

Choose a reason for hiding this comment

tjenkinson Sep 5, 2020

Choose a reason for hiding this comment

tjenkinson commented Oct 11, 2020

danielgindi commented Oct 11, 2020

tjenkinson commented Oct 11, 2020

danielgindi commented Oct 11, 2020

tjenkinson commented Oct 11, 2020

danielgindi commented Oct 11, 2020

LarsDenBakker commented Oct 11, 2020

tjenkinson commented Oct 11, 2020

LarsDenBakker commented Oct 11, 2020

tjenkinson commented Oct 11, 2020 • edited

lukastaegert commented Oct 13, 2020

tjenkinson commented Oct 13, 2020

lukastaegert commented Oct 14, 2020

tjenkinson commented Oct 14, 2020

lukastaegert commented Oct 15, 2020

tjenkinson commented Oct 24, 2020

shellscape commented Oct 25, 2020

tjenkinson commented Nov 1, 2020

tjenkinson commented Sep 5, 2020 •

edited

Rollup Plugin Name: `node-resolve`

tjenkinson commented Oct 11, 2020 •

edited