Fix passthrough for dynamic import

[jdaugherty]

Fixes #3941

@brad4d since you added a comment asking for help, I've created this PR. I took the approach you suggested - having a separate method that silently errors.

[brad4d]

I've imported this PR for testing and review.

[brad4d]

@ChadKillingsworth, could you take a look at this PR and make sure it works for you, since you are likely to be impacted by these changes?

[jdaugherty]

@brad4d I think this is a bug looking at how the other modules implement resolveJsModule. I'm assuming transformedAddress == null should really be loadAddress == null ? Earlier in this method a null pointer would have already been thrown if transformedAddress & a prefix replacement was defined too.

[jdaugherty]

@brad4d I pushed a commit to help de-duplicate the copied logic. Hopefully this makes it clearer that resolveJsModuleSilently is effectively a copy each ModuleResolver's implementation, without the error. If you want me to rename the method, I'm happy to.

I also pushed a change to where I assume the wrong value was being checked:

https://github.com/jdaugherty/closure-compiler/blob/5ebabe0e2b823c8f16223030380b691dad75309a/src/com/google/javascript/jscomp/deps/BrowserWithTransformedPrefixesModuleResolver.java#L150

[brad4d]

I'm comfortable with the way the code looks now.
I've just sent it off for testing.

[brad4d]

Alas, stuff broke.
I'll have to look into this further next week.

[jdaugherty]

@brad4d Thank you. It may be because I thought https://github.com/jdaugherty/closure-compiler/blob/5ebabe0e2b823c8f16223030380b691dad75309a/src/com/google/javascript/jscomp/deps/BrowserWithTransformedPrefixesModuleResolver.java#L150 was a bug - if it's not, I can revert that change. If you can help point me to what broke, I'm happy to help resolve these issues too.

[ChadKillingsworth]

@brad4d It's been a long time since I've scrutinized this code, so its going to be hard for me to review this without spending a lot of time getting re-familiar with it. I am going to run this on my own project that makes heavy use of dynamic imports and see what happens.

However - it looks like one of the tests is failing so I can't get a build artifact created to run that test.

[jdaugherty]

@ChadKillingsworth @brad4d Apologies, I thought the tests were working locally. Let me take another pass at this and see where my dedupe caused the cause.

[jdaugherty]

So the failing test (Es6RelativizeImportPathsTest) is failing because of the fix for #4298 (comment)

I'm assuming that the right decision is to leave the fix in place (since that's how the other module resolvers work) and just ignore the LOAD_WARNING warning in the test. This is a behavior change though - before these errors were being swallowed / hidden.

For the test itself, the module address raw0/foo.js is being transformed to /mapped0/foo.js which isn't a valid module path. If you think this is the right behavior, I can revert that if check and they'll pass.

@brad4d @ChadKillingsworth Should I revert that fix or should I update the test to ignoreWarnings(LOAD_WARNING)?

[jdaugherty]

The change being questioned is this one: jdaugherty@5ebabe0

[brad4d]

It appears that quite a few things within Google depend on the apparent bug that ignores a failed module load here.

closure-compiler/src/com/google/javascript/jscomp/deps/BrowserWithTransformedPrefixesModuleResolver.java

Line 146 in fa09c83

if (transformedAddress == null) {

So, the test failing within the closure-compiler repository when we "fix" it is the tip of the iceberg.

I have:

1. Filed an internal issue about this apparent bug and started investigating how we could either fix it or determine that it's not actually a bug, since things have been overall "working" for us since 2018 when the bug was introduced.
2. Modified the change I imported to "unfix" the bug, so it won't block me merging this PR.

I'm about to send this modification for internal testing.

[jdaugherty]

Thank you @brad4d , I'm here if I can help ...

[brad4d]

Good news! It looks like all the tests passed once I unfixed the "obvious bug".
I'm sending the change to another engineer for review.

p.s. I suspect that the bug is hiding a case where we actually need the same "silent failure" behavior, but I'm not sure, and there's no need to fix it before merging this.