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

feat(compat): add .code to dyn import error #12633

Merged
merged 7 commits into from
Nov 8, 2021

Conversation

kt3k
Copy link
Member

@kt3k kt3k commented Nov 3, 2021

part of #12577, closes #12588

This PR changes the error class for dynamic import rejection in compat mode. Some tools (e.g. mocha) uses .code property of error object to detect the resolution failure of dynamic import (ref: https://github.com/mochajs/mocha/blob/97b84708afb42e552cb906a54f9f2aa2e6a98ba4/lib/nodejs/esm-utils.js#L47-L56 ). This change simulates the above behavior in compat mode.

This change enables compat mode to run the basic feature of mocha cli

@kt3k kt3k force-pushed the feat/swap-dyn-import-error-class branch from a5dd20c to 7cda7c3 Compare November 3, 2021 12:32
@kt3k
Copy link
Member Author

kt3k commented Nov 3, 2021

cli/tests/testdata/fix_dynamic_import_errors.js test case strangely panicks at the 2nd dynamic import..

@kt3k kt3k force-pushed the feat/swap-dyn-import-error-class branch from 7cda7c3 to 8529adc Compare November 3, 2021 12:34
@kt3k kt3k force-pushed the feat/swap-dyn-import-error-class branch from 8529adc to a4aec92 Compare November 3, 2021 12:55
@kt3k
Copy link
Member Author

kt3k commented Nov 4, 2021

Now ready for review

I first tried to reuse class_name of CustomError for accessing the custom js error constructor, but that didn't work because some errors cause panic during getting error class (caused by cli/tests/testdata/fix_dynamic_import_errors.js). Now I created a dedicated error class for that purpose CustomErrorWithJsConstructor and retrieves js constructor name from that class during rejections of dynamic imports

@bartlomieju
Copy link
Member

Please discuss this change in the design letting before landing.

@lucacasonato lucacasonato self-requested a review November 4, 2021 13:56
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not in favor of this Deno.compat.errors namespace. In node await import("xxxxxx") throws a regular Error (no subclass), with a .code on it. In the browser it throws a TypeError. I think we should always throw a TypeError (also in non-compat mode), and in compat mode add a code property to that.

@kt3k kt3k changed the title feat(compat): switch dyn import error class feat(compat): add .code to dyn import error Nov 7, 2021
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kt3k kt3k merged commit 8e010b6 into denoland:main Nov 8, 2021
@kt3k kt3k deleted the feat/swap-dyn-import-error-class branch November 8, 2021 07:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants