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: allow edit of commonJs plugin params #187

Merged
merged 6 commits into from
May 4, 2022

Conversation

huang-julien
Copy link
Contributor

@huang-julien huang-julien commented May 3, 2022

  • add commonjs field to edit the commonjs plugin

πŸ”— Linked issue

nuxt/framework#4762
nuxt/nuxt#13843

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Add the commonJs key to NitroOptions and assign by destructuring nitro.option.commonJs when adding the @rollup/plugin-commonjs.
Would improve the linked PR by avoiding replacing the commonJS plugin at build. The commonJS dynamicRequireTargets declaration would be directly into nitro/nuxt config

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

- add commonjs field to edit the commonjs plugin
@@ -139,6 +140,7 @@ export interface NitroOptions {
externals: NodeExternalsOptions
analyze: false | PluginVisualizerOptions
replace: Record<string, string | ((id: string) => string)>
commonJs?: RollupCommonJSOptions
Copy link
Member

Choose a reason for hiding this comment

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

Can you please use commonJS for naming?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done πŸ‘

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Thanks! Would be nice to also update docs (config reference to mention this new option)

docs/config/index.md Outdated Show resolved Hide resolved
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

3 participants