[dynamic-import-vars] Give default promise rejection timing parity with cases #825

bearfriend · 2021-03-03T17:05:43Z

Rollup Plugin Name: `dynamic-import-vars`

This PR contains:

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

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

List any relevant issue numbers:

Description

The immediately rejected promise in the default clause of the generated switch does not have timing parity with the other clauses, making it impossible to register a catch on the promise before it is rejected, which causes debuggers with "pause on exceptions" (but not "pause on caught exceptions") enabled to always pause in the default clause, even if you have registered a catch on the returned promise. This queues the promise rejection as a microtask to remedy that situation.

…rity with cases

shellscape · 2021-03-10T15:22:49Z

@LarsDenBakker it looks like this has some community support. Please have a look over this when you have the opportunity.

LarsDenBakker · 2021-03-10T15:38:49Z

This changes the natural behavior of dynamic imports, I'm not sure if it's a good idea.

For example, given:

import(`./pages/${page}.js`);

If the requested file does not exist, the browser will throw an error before bundling with rollup. Why should it not throw an error after bundling with rollup?

If your code is intended to ignore/handle 404s, you can do this:

try {
  await import(`./pages/${page}.js`);
} catch (error) {
  // handle the error
}

That way the code behaves the same both before and after bundling.

bearfriend · 2021-03-10T17:56:32Z

Perhaps it should (optionally) throw an error after bundling, but if it does, it should do so asynchronously, as the 404 error would be.

The reason it would be nice to be optional is that in the actual code that is run after bundling, there is no natural error. Nothing has been attempted and nothing has failed. If you loop through a set of variables until one matches, as we do (and one is guaranteed to match), it would be nice not to have to catch a pointless error.

Also note that your example, as written, will never catch as the promise is returned successfully, even if it will be rejected. Even if you await the import, after bundling the error occurs in the scope of the generated function and it will throw.

LarsDenBakker · 2021-03-10T20:53:22Z

You're right I'm missing an await in my example, I updated it.

Improving the way we handle the rejected promise is fine by me, I wasn't aware we didn't achieve this already with the current code.

I'm personally still not convinced of the use case you describe. If there is high demand for it, we can add it. But as far as I can see it's a few lines of code to handle the catch yourself and it makes the code more explicit and work without bundling as well...

bearfriend · 2021-03-10T21:39:41Z

I think that's fair. If we queue the promise rejection as a microtask, making the default clause optional becomes less necessary, and it can be handled in a catch much more effectively. I probably should've split the methods into separate PRs to begin with.

Should I go ahead and revert the defaultClause code?

LarsDenBakker · 2021-03-10T21:50:39Z

Sure, that would be fine!

tjenkinson · 2021-03-14T16:35:42Z

packages/dynamic-import-vars/README.md

-      throw new Error('Unknown variable dynamic import: ' + path);
+      return new Promise((resolve, reject) => {
+        queueMicrotask(() => {
+          reject(new Error("Unknown variable dynamic import: " + path))


Sorry not reviewed this in detail, but wondering if queueMicrotask is supported in most places or if we should use something a bit older instead to achieve this?

Would a standard return Promise.reject() not work?

It would not. The actual code falls back to setTimeout.

Looked in a bit more detail.

What wouldn’t work with return Promise.reject(new Error("Unknown variable dynamic import: " + path)) work?

The return is always a promise then which would resolve/reject async. I understand why the sync throw before was not great

The code actually already is using Promise.reject. The documentation just doesn't match. Please run this with your dev tools open and Pause on exception enabled to see what this fixes.

https://jsfiddle.net/g6u4femL/

Note that even though both promises are caught, it will still break on promiseA.

Ah ok thanks for explaining.

Seems strange there’s not a simpler way but 👍

Feels like this is something dev tools should handle? If you’re pausing on uncaught exceptions I wonder why it doesn’t wait a tick internally until the point when it actually rejects 🤔

tjenkinson

Makes sense to me, but I think we could just return a Promise.reject(...) and solve the problem with less code, so that would be preferable IMO

tjenkinson

Sorry just spotted something else

tjenkinson · 2021-03-15T21:21:27Z

packages/dynamic-import-vars/src/index.js

-   }
+  switch (path) {
+${paths.map((p) => `    case '${p}': return import('${p}');`).join('\n')}
+${`    default: return new Promise((resolve, reject) => {


Should we use a standard function in here vs the arrow one?

I suppose an arrow function isn't strictly necessary. Do you see a downside to it in this case? There's no scope it's ignoring.

It depends what browsers this is targeting. Promises are polyfillable but the arrow syntax isn’t, so it would be safer to stick with the older function format I think.

Ah ok. Didn't realize we were going back that far with browser support, but yeah I guess it's the safest thing to do and no real downside.

bearfriend · 2021-03-18T20:38:41Z

Anything blocking this?

tjenkinson · 2021-03-26T19:24:36Z

Looks good to go but @shellscape usually does the merge/release when they get chance :)

shellscape · 2021-03-26T23:04:51Z

@tjenkinson put your green checkmark on it and I'll merge it post haste

bearfriend force-pushed the default-clause branch 2 times, most recently from 044210c to 7652817 Compare March 3, 2021 17:48

Allow the default clause to be disabled; When enabled, give timing pa…

6d07141

…rity with cases

bearfriend force-pushed the default-clause branch from 7652817 to 6d07141 Compare March 4, 2021 00:28

shellscape requested a review from LarsDenBakker March 10, 2021 15:22

shellscape added the pr³ 🔧 feature label Mar 10, 2021

Revert defaultClause option

4247bf4

bearfriend changed the title ~~[dynamic-import-vars] Allow default clause to be disabled and give timing parity with cases~~ [dynamic-import-vars] Give default promise rejection timing parity with cases Mar 10, 2021

LarsDenBakker approved these changes Mar 14, 2021

View reviewed changes

tjenkinson reviewed Mar 14, 2021

View reviewed changes

tjenkinson requested changes Mar 15, 2021

View reviewed changes

tjenkinson approved these changes Mar 15, 2021

View reviewed changes

tjenkinson reviewed Mar 15, 2021

View reviewed changes

Avoid arrow functions

db8d07c

bearfriend requested a review from tjenkinson March 26, 2021 18:36

tjenkinson approved these changes Mar 27, 2021

View reviewed changes

shellscape merged commit c328e2c into rollup:master Mar 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[dynamic-import-vars] Give default promise rejection timing parity with cases #825

[dynamic-import-vars] Give default promise rejection timing parity with cases #825

bearfriend commented Mar 3, 2021 •

edited

shellscape commented Mar 10, 2021

LarsDenBakker commented Mar 10, 2021 •

edited

bearfriend commented Mar 10, 2021 •

edited

LarsDenBakker commented Mar 10, 2021 •

edited

bearfriend commented Mar 10, 2021 •

edited

LarsDenBakker commented Mar 10, 2021

tjenkinson Mar 14, 2021

bearfriend Mar 15, 2021

tjenkinson Mar 15, 2021

bearfriend Mar 15, 2021

tjenkinson Mar 15, 2021

tjenkinson Mar 15, 2021

tjenkinson left a comment

tjenkinson left a comment

tjenkinson Mar 15, 2021

bearfriend Mar 15, 2021

tjenkinson Mar 15, 2021

bearfriend Mar 15, 2021

bearfriend commented Mar 18, 2021

tjenkinson commented Mar 26, 2021

shellscape commented Mar 26, 2021

[dynamic-import-vars] Give default promise rejection timing parity with cases #825

[dynamic-import-vars] Give default promise rejection timing parity with cases #825

Conversation

bearfriend commented Mar 3, 2021 • edited

Rollup Plugin Name: dynamic-import-vars

Description

shellscape commented Mar 10, 2021

LarsDenBakker commented Mar 10, 2021 • edited

bearfriend commented Mar 10, 2021 • edited

LarsDenBakker commented Mar 10, 2021 • edited

bearfriend commented Mar 10, 2021 • edited

LarsDenBakker commented Mar 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjenkinson left a comment

Choose a reason for hiding this comment

tjenkinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bearfriend commented Mar 18, 2021

tjenkinson commented Mar 26, 2021

shellscape commented Mar 26, 2021

bearfriend commented Mar 3, 2021 •

edited

Rollup Plugin Name: `dynamic-import-vars`

LarsDenBakker commented Mar 10, 2021 •

edited

bearfriend commented Mar 10, 2021 •

edited

LarsDenBakker commented Mar 10, 2021 •

edited

bearfriend commented Mar 10, 2021 •

edited