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.

Sign up for GitHub

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

Jump to bottom

feat: mark files in a node_modules as ignore-listed by default #4877

Merged
merged 1 commit into from
Mar 1, 2023
Merged

feat: mark files in a node_modules as ignore-listed by default #4877

merged 1 commit into from
Mar 1, 2023

Conversation

bmeurer
Copy link
Contributor

@bmeurer bmeurer commented Feb 24, 2023 •
edited
Loading

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 •
edited
Loading

✅ 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 •
edited
Loading

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
lukastaegert
lukastaegert requested changes Feb 25, 2023
View reviewed changes
Copy link
Member

@lukastaegert lukastaegert left a comment •
edited
Loading

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 bmeurer mentioned this pull request Feb 27, 2023
Merged
9 tasks
@bmeurer bmeurer force-pushed the feat/ignoreList_node_modules_default branch from ed11971 to 931b43b Compare February 28, 2023 10:42
@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!

@bmeurer bmeurer requested a review from lukastaegert March 1, 2023 09:08
@bmeurer @lukastaegert
feat: mark files in a node_modules as ignore-listed by default
ca3eba9 
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 disabled auto-merge March 1, 2023 18:22
@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
Reviewers

@lukastaegert lukastaegert lukastaegert approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bmeurer @rollup-bot @lukastaegert