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

Default --experimental-detect-module to commonjs in require hooks #53016

Closed
RedYetiDev opened this issue May 15, 2024 · 3 comments
Closed

Default --experimental-detect-module to commonjs in require hooks #53016

RedYetiDev opened this issue May 15, 2024 · 3 comments
Labels
loaders Issues and PRs related to ES module loaders module Issues and PRs related to the module subsystem.

Comments

@RedYetiDev
Copy link
Member

RedYetiDev commented May 15, 2024

When detect-module is enabled by default (as seen in this image), we're encountering failures in several tests from test/es-module. While most of these failures are expected due to the changes, two of them are not:

These errors seem to be related to how the format context property is defined. We've attempted various solutions, but we're seeking input on the next steps on how to properly define this property.

Some ideas we're considering:

  1. Ensure format is always defined: We could make sure format is never undefined, even if we don't know what it should be.

    • Pros:
      • Eliminates concerns about different edge cases for return types.
      • Avoids the need to redefine format multiple times internally.
    • Cons:
      • If we don't know the format, what value should we set it to?
  2. Distinguish null and undefined formats: Specifically for this use case, we could differentiate between null and undefined formats.

    • Pros:
      • Could work well for our purposes.
      • Requires minimal effort to implement.
    • Cons:
      • Maintenance could be challenging, as future changes would need to understand this distinction.

Do you have any insights or suggestions on these approaches, or perhaps an alternative solution we haven't considered?

@RedYetiDev RedYetiDev added help wanted Issues that need assistance from volunteers or PRs that need help to proceed. module Issues and PRs related to the module subsystem. loaders Issues and PRs related to ES module loaders labels May 15, 2024
@mews6
Copy link

mews6 commented May 18, 2024

Would this be a good first issue?

@RedYetiDev
Copy link
Member Author

Personally, I don't think this would be a good first issue, as it's a complex issue to tackle, but you can always try it.

Most good first issues have been labeled as "good first issue"

@RedYetiDev RedYetiDev removed the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label May 19, 2024
@RedYetiDev
Copy link
Member Author

(Will be) Fixed by #53044

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
loaders Issues and PRs related to ES module loaders module Issues and PRs related to the module subsystem.
Projects
None yet
Development

No branches or pull requests

2 participants