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

Add resolved sources to shouldTransformCachedModule #4414

Merged
merged 6 commits into from Feb 22, 2022

Conversation

lukastaegert
Copy link
Member

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:
rollup/plugins#1038

Description

Another small addition to finalize the new commonjs plugin. In order to figure out if cached resolved ids changed, I need to add them to the shouldTransformCachedModule hook.

@github-actions
Copy link

github-actions bot commented Feb 20, 2022

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install rollup/rollup#should-transform-dependencies

or load it into the REPL:
https://rollupjs.org/repl/?pr=4414

@codecov
Copy link

codecov bot commented Feb 20, 2022

Codecov Report

Merging #4414 (e2e4d72) into master (3106ec9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4414   +/-   ##
=======================================
  Coverage   98.75%   98.75%           
=======================================
  Files         204      204           
  Lines        7322     7322           
  Branches     2079     2079           
=======================================
  Hits         7231     7231           
  Misses         33       33           
  Partials       58       58           
Impacted Files Coverage Δ
src/Module.ts 100.00% <ø> (ø)
src/ModuleLoader.ts 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3106ec9...e2e4d72. Read the comment docs.

@lukastaegert lukastaegert merged commit 7cc87f1 into master Feb 22, 2022
@lukastaegert lukastaegert deleted the should-transform-dependencies branch February 22, 2022 06:12
@frank-dspeed
Copy link
Contributor

frank-dspeed commented Feb 27, 2022

@lukastaegert sorry for posting that here but i found this commit and i am wondering because i saw that in a microsoft project and it was more a feature then a bug or anything else can there be a flag to store both? in the cache? so that the cache has the transformed and untransformed source.

i guess that sounds strange for you as you will arrgument the files are already there why should we store them again?

because the structure of the directorys changes and that shows me easy and good programatical process able where deep resolve problems do occure. (Resolve problems that do not produce errors) Eg: Wrong file resolved.

also i am trying to turn rollup into the foundation of a universal loader like systemjs for modern environments that do not support diffrent resolve algos.

Short story

can there be a option to also store untransformed sources in the cache dir?

Short why

Big incremental builds with a lot of unknown 3rd party dependencies (CDN Provider)

@lukastaegert
Copy link
Member Author

When you refer to the "cache dir", what are you talking about? If you refer to Rollup's cache option, the untranspiled source IS part of the cache because it is used to check after a loader if we want to transform again.

@frank-dspeed
Copy link
Contributor

@lukastaegert ok perfect was not sure if that is related thanks for the information i love that.

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

2 participants