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: mark files in a node_modules as ignore-listed by default #4877

Merged

Conversation

bmeurer
Copy link
Contributor

@bmeurer bmeurer commented Feb 24, 2023

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

#4847

Description

Following up on the addition of the sourcemapIgnoreList output option, this adds a default which marks all files within a node_modules folder as ignore-listed. This way most projects using rollup will need no additional configuration regarding the ignore-listing and automatically benefit.

Doc: https://goo.gle/devtools-ignoreList-adoption

@netlify
Copy link

netlify bot commented Feb 24, 2023

Deploy Preview for rollupjs ready!

Name Link
🔨 Latest commit ca3eba9
🔍 Latest deploy log https://app.netlify.com/sites/rollupjs/deploys/63ff942e05ef760008f68c18
😎 Deploy Preview https://deploy-preview-4877--rollupjs.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 settings.

@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Merging #4877 (ca3eba9) into master (1ab9833) will increase coverage by 0.00%.
The diff coverage is 91.66%.

@@           Coverage Diff           @@
##           master    #4877   +/-   ##
=======================================
  Coverage   98.98%   98.98%           
=======================================
  Files         219      219           
  Lines        7948     7951    +3     
  Branches     2189     2190    +1     
=======================================
+ Hits         7867     7870    +3     
  Misses         26       26           
  Partials       55       55           
Impacted Files Coverage Δ
src/utils/renderChunks.ts 99.10% <87.50%> (-0.01%) ⬇️
src/utils/options/normalizeOutputOptions.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@bmeurer bmeurer force-pushed the feat/ignoreList_node_modules_default branch 2 times, most recently from d5509d6 to ed11971 Compare February 24, 2023 13:02
Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Thank you!

While I approve the change in general, now the only way to switch it off is to pass a function that always returns false.
I think we should adjust the types and handling slightly:

  • E.g. allow an explicit false to turn off the feature. That would be consistent with how this works for other options like treeshake or watch
  • Either translate false to an "always false" predicate and have always a function in the normalizedOutputOptions, or hand through the false value. I am slightly leaning to the former approach of always having a function for simplicity. In that case, we can remove the conditional logic in renderChunks that is currently not covered.

And of course, documentation should be adapted to show the new types and explain the default logic.

@bmeurer
Copy link
Contributor Author

bmeurer commented Feb 28, 2023

Thank you!

While I approve the change in general, now the only way to switch it off is to pass a function that always returns false. I think we should adjust the types and handling slightly:

  • E.g. allow an explicit false to turn off the feature. That would be consistent with how this works for other options like treeshake or watch
  • Either translate false to an "always false" predicate and have always a function in the normalizedOutputOptions, or hand through the false value. I am slightly leaning to the former approach of always having a function for simplicity. In that case, we can remove the conditional logic in renderChunks that is currently not covered.

And of course, documentation should be adapted to show the new types and explain the default logic.

Done, thanks!

Following up on the addition of the `sourcemapIgnoreList` output option,
this adds a default which marks all files within a `node_modules` folder
as ignore-listed. This way most projects using rollup will need no
additional configuration regarding the ignore-listing and automatically
benefit.

This also allows to specify `false` for the `sourcemapIgnoreList` output
option, which completely disables the generation of an ignore-list.

Doc: https://goo.gle/devtools-ignoreList-adoption
@lukastaegert lukastaegert force-pushed the feat/ignoreList_node_modules_default branch from 931b43b to ca3eba9 Compare March 1, 2023 18:06
@lukastaegert lukastaegert enabled auto-merge (squash) March 1, 2023 18:06
@lukastaegert lukastaegert merged commit 3d9d107 into rollup:master Mar 1, 2023
@rollup-bot
Copy link
Collaborator

This PR has been released as part of rollup@3.18.0. You can test it via npm install rollup.

@bmeurer bmeurer deleted the feat/ignoreList_node_modules_default branch March 2, 2023 05:56
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.

None yet

3 participants