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!: align AssetGeneratorDataUrlFunction with webpack #8614

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

inottn
Copy link
Collaborator

@inottn inottn commented Dec 4, 2024

Summary

close #8538

This is a breaking change, but the previous behavior was inconsistent with webpack.

Checklist

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

Copy link

netlify bot commented Dec 4, 2024

Deploy Preview for rspack ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b5c2f02
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/6760336f85e17c0008a4a10d
😎 Deploy Preview https://deploy-preview-8614--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.

@github-actions github-actions bot added the release: feature release: feature related release(mr only) label Dec 4, 2024
Copy link
Member

@chenjiahan chenjiahan left a comment

Choose a reason for hiding this comment

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

How about be compatible with the previous API signature to avoid breaking the user's code?

For example, if arg is an object, convert it to source + context.

We can align type declarations with webpack, but the internal implementation is compatible with Rspack <= 1.1.5. The compatible code can be removed in Rspack v2.0.

@inottn
Copy link
Collaborator Author

inottn commented Dec 7, 2024

How about be compatible with the previous API signature to avoid breaking the user's code?

For example, if arg is an object, convert it to source + context.

We can align type declarations with webpack, but the internal implementation is compatible with Rspack <= 1.1.5. The compatible code can be removed in Rspack v2.0.

Sorry for the delay. If the parameters were directly passed to us by the user, we could maintain compatibility with the old version. However, in this case, the user provides a dataUrl function, and we pass the parameters to it, making compatibility with the old version unfeasible. Let’s put this PR on hold for now and merge it at a more suitable time.

@chenjiahan
Copy link
Member

This is not a very common usage, and it makes more sense to align with webpack. Maybe we can include this change in Rspack v1.2.0. @hardfist what do you think

@inottn inottn force-pushed the feat/generator-data-url branch 2 times, most recently from 8f987a1 to 750fc12 Compare December 14, 2024 13:11
@chenjiahan
Copy link
Member

@hardfist cc

@chenjiahan
Copy link
Member

After discussion, we decided to include this PR in v1.2.0 to align with webpack.

@inottn Can you rebase the PR again~ ❤️

@inottn inottn force-pushed the feat/generator-data-url branch from 750fc12 to b5c2f02 Compare December 16, 2024 14:04
@inottn
Copy link
Collaborator Author

inottn commented Dec 16, 2024

After discussion, we decided to include this PR in v1.2.0 to align with webpack.

@inottn Can you rebase the PR again~ ❤️

done~

Copy link
Member

@chenjiahan chenjiahan left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@chenjiahan chenjiahan merged commit fb2869d into web-infra-dev:main Dec 16, 2024
31 checks passed
@inottn inottn deleted the feat/generator-data-url branch December 16, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: feature release: feature related release(mr only)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: AssetGeneratorDataUrlFunction should align with webpack
2 participants