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

Draft: fix: align amd option behavior with webpack #9011

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

nilptr
Copy link
Contributor

@nilptr nilptr commented Jan 14, 2025

Summary

closes #8987

  • fix: align amd option behavior with webpack
  • chore: add doc for amd option

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

Copy link

netlify bot commented Jan 14, 2025

Deploy Preview for rspack ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 7431665
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/678a7885ad7fa7000718f343
😎 Deploy Preview https://deploy-preview-9011--rspack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -372,7 +372,7 @@ export const getNormalizedRspackOptions = (
watchOptions: cloneObject(config.watchOptions),
devServer: config.devServer,
profile: config.profile,
amd: config.amd ? JSON.stringify(config.amd) : undefined,
amd: config.amd !== false ? JSON.stringify(config.amd || {}) : undefined,
Copy link
Contributor Author

@nilptr nilptr Jan 14, 2025

Choose a reason for hiding this comment

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

webpack does not explicitly specify a default value for amd option. however, undefined is equivalent to {} in the implementation. ref

this change ensures the behavior is consistent with webpack. but i'm wondering if we should explicitly state the default value is {}. what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this line to getRawOptions.

Copy link

codspeed-hq bot commented Jan 14, 2025

CodSpeed Performance Report

Merging #9011 will not alter performance

Comparing nilptr:nilptr/refactor/amd-doc (7431665) with main (c47c243)

Summary

✅ 3 untouched benchmarks

@nilptr nilptr changed the title align amd option behavior with webpack and add doc for amd option fix: align amd option behavior with webpack Jan 15, 2025
@github-actions github-actions bot added the release: bug fix release: bug related release(mr only) label Jan 15, 2025
@nilptr nilptr changed the title fix: align amd option behavior with webpack Draft: fix: align amd option behavior with webpack Jan 15, 2025
@github-actions github-actions bot removed the release: bug fix release: bug related release(mr only) label Jan 15, 2025
@nilptr nilptr force-pushed the nilptr/refactor/amd-doc branch 2 times, most recently from 415b69c to a13a6da Compare January 16, 2025 15:45
_expr: &swc_core::ecma::ast::CallExpr,
for_name: &str,
) -> Option<bool> {
(parser.is_esm && is_non_esm_identifier(for_name)).then_some(true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the module is an esm, define/typeof define/define(...) processing will be skipped by HarmonyDetectionParserPlugin.

@nilptr nilptr force-pushed the nilptr/refactor/amd-doc branch from a13a6da to c0460c7 Compare January 17, 2025 01:53
@nilptr nilptr force-pushed the nilptr/refactor/amd-doc branch from c0460c7 to 7431665 Compare January 17, 2025 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tracking]: Lack of documentation for AMD configuration
1 participant