fix(compartment-mapper): Defer all importHook errors when importing a… #2610
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… dependency if they're intended to be deferred
naugtur
commented
Oct 23, 2024
| return deferError( | ||
| moduleSpecifier, | ||
| Error( | ||
| throw Error( |
There was a problem hiding this comment.
returning instead of throwing could be slightly more performant
naugtur
commented
Oct 23, 2024
| @@ -502,89 +502,88 @@ export const makeImportHookMaker = ( | |||
| // for lint rule | |||
| await null; | |||
There was a problem hiding this comment.
This diff looks much better with whitespace hidden
boneskull
approved these changes
Oct 24, 2024
Comment on lines
+577
to
+586
| throw Error( | ||
| `Cannot find file for internal module ${q( | ||
| moduleSpecifier, | ||
| )} (with candidates ${candidates | ||
| .map(x => q(x)) | ||
| .join(', ')}) in package ${packageLocation}`, | ||
| ), | ||
| ); | ||
| ); | ||
| } catch (error) { | ||
| return deferError(moduleSpecifier, error); | ||
| } |
There was a problem hiding this comment.
IMO this should probably just return the deferError instead of throwing and catching it immediately, but if there's a good reason for doing this then ok.
There was a problem hiding this comment.
no good reason, just being DRY/consistent. Didn't feel good doing it.
kriskowal
requested changes
Oct 24, 2024
|
@kriskowal Reconsider? This looks like a bugfix to me. |
boneskull
added a commit
that referenced
this pull request
Oct 30, 2024
For CJS compat, the top-level `this` must be equal to `module.exports` (which must be equal to `exports`). This implementation changes the CJS parsers to call the wrapper `functor` using the context of `module.exports`. (This PR targets #2610)
boneskull
added a commit
that referenced
this pull request
Oct 30, 2024
For CJS compat, the top-level `this` must be equal to `module.exports` (which must be equal to `exports`). This implementation changes the CJS parsers to call the wrapper `functor` using the context of `module.exports`. (This PR targets #2610)
kriskowal
approved these changes
Oct 31, 2024
|
Consider adding to NEWS. |
boneskull
added a commit
that referenced
this pull request
Nov 4, 2024
For CJS compat, the top-level `this` must be equal to `module.exports` (which must be equal to `exports`). This implementation changes the CJS parsers to call the wrapper `functor` using the context of `module.exports`. (This PR targets #2610)
# By Turadg Aleahmad (11) and others # Via GitHub (4) and Kris Kowal (1) * master: docs(ses): Finish embracing permits terminology fix(ses): Clarify lockdown types feat(ses): Lockdown reporting option chore(deps): yarn dedupe chore(types): fix import tags lint: enable jsdoc/recommended-typescript-flavor chore: rm dup config key chore(deps): bump eslint-plugin-jsdoc build(deps): bump typescript (patch) chore(types): conform to verbatimModuleSyntax ci: enable verbatimModuleSyntax style: prettier chore(deps): bump prettier build(deps): bump yarn to 4.5.1 feat(compartment-mapper): Collect unretained module descriptors test(compartment-mapper): Mark and sweep unused module descriptors feat(ses): permit Promise.prototype.try (#2609) # Conflicts: # packages/compartment-mapper/NEWS.md
boneskull
added a commit
that referenced
this pull request
Nov 4, 2024
For CJS compat, the top-level `this` must be equal to `module.exports` (which must be equal to `exports`). This implementation changes the CJS parsers to call the wrapper `functor` using the context of `module.exports`. (This PR targets #2610)
|
I've updated and added a blurb to |
kriskowal
approved these changes
Nov 4, 2024
boneskull
added a commit
that referenced
this pull request
Nov 4, 2024
For CJS compat, the top-level `this` must be equal to `module.exports` (which must be equal to `exports`). This implementation changes the CJS parsers to call the wrapper `functor` using the context of `module.exports`. (This PR targets #2610)
boneskull
added a commit
that referenced
this pull request
Nov 5, 2024
For CJS compat, the top-level `this` must be equal to `module.exports` (which must be equal to `exports`). This implementation changes the CJS parsers to call the wrapper `functor` using the context of `module.exports`. (This PR targets #2610)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
… dependency if they're intended to be deferred
Description
When importHook is used to load dependencies, they might be coming from
importsthat were identified by a parser that uses heuristics to find them. (e.g. cjs parser implementation uses a lexer to find allrequirecalls even if they're not the require function but a local require function)In such cases we want to defer loading errors to runtime, so the error doesn't happen until actual
requireis actually called to get it.That was true for loading known dependency, but exit modules (via exitModuleImportHook) were not covered.
Security Considerations
no changes security-wise
Scaling Considerations
The way this is currently implemented - one big try-catch instead of returning the deferred error without throwing and catching, might be less performant. Should we consider trading aesthetics for a tiny performance gain here?
Note this only makes a difference for cases where non-dynamic
requireis called on a specifier that causes an error.Documentation Considerations
It's a fix of unintended behavior. No changes to what needs to be documented.
Testing Considerations
Fix was added test-first
Compatibility Considerations
Compatibility is improved. Packages can now do:
to silently attempt to load something that is only useful in their own development process. (typescript does that)
Upgrade Considerations
While it's a fix, not a breaking change, it does change one specific behavior - an error resulting from calling exitModuleImportHook (the
importHookpassed to compartment-mapper, not the one in Compartment options) is now thrown at runtime, not during loading, which could be noteworthy for the Archive use-case.(insert mandatory xkcd about breaking changes)