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

fix(externals): fallback traced files for minor/patch versions #529

Merged
merged 3 commits into from
Oct 11, 2022

Conversation

danielroe
Copy link
Member

πŸ”— Linked issue

resolves #526, nuxt/nuxt#14995

❓ 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

Currently, if we trace file a from v 7.1 and file b from v 7.0 of a putative package, we discard b entirely. This PR changes the behaviour to copy across b from v 7.1. This isn't a perfect fix as we stil may have issues with major version differences, but it can resolve small differences, for example in nuxt/nuxt#14995.

πŸ“ Checklist

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

@danielroe danielroe added the bug Something isn't working label Sep 23, 2022
@danielroe danielroe requested a review from pi0 September 23, 2022 12:34
@danielroe danielroe self-assigned this Sep 23, 2022
@codecov
Copy link

codecov bot commented Sep 23, 2022

Codecov Report

Merging #529 (19cbb44) into main (8a82de7) will increase coverage by 0.01%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main     nuxt/bridge#106      +/-   ##
==========================================
+ Coverage   66.09%   66.11%   +0.01%     
==========================================
  Files          55       55              
  Lines        4056     4061       +5     
  Branches      438      441       +3     
==========================================
+ Hits         2681     2685       +4     
- Misses       1371     1372       +1     
  Partials        4        4              
Impacted Files Coverage Ξ”
src/rollup/plugins/externals.ts 88.19% <85.71%> (-0.16%) ⬇️

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

@danielroe danielroe changed the title fix: fallback traced files for minor/patch versions fix(externals): fallback traced files for minor/patch versions Oct 11, 2022
@danielroe danielroe merged commit d90c938 into main Oct 11, 2022
@danielroe danielroe deleted the fix/trace-minors branch October 11, 2022 13:09
@pi0
Copy link
Member

pi0 commented Oct 12, 2022

I'm reverting this PR. Mixing two files of two different package versions is not a nice idea!

At least for mentioned node-fetch-native, we might find a better solution and for other possible cases, think of supporting hosting for trace extraction.

pi0 added a commit that referenced this pull request Oct 12, 2022
@pi0 pi0 mentioned this pull request Oct 12, 2022
pi0 added a commit that referenced this pull request Oct 12, 2022
This reverts commit d90c938.
@danielroe
Copy link
Member Author

Very happy for this to be reverted. It was only meant to be a hotfix. The current externals tracing implementation is fairly fragile; even differing patch versions of a package may cause 500 errors on deploy.

There are also other issues regarding differing major versions.

Probably all of these can be solved by reworking the implementation.

@pi0
Copy link
Member

pi0 commented Oct 12, 2022

Indeed it is fraglie and broken if internal structure of package changes in semver/minor.. Moved discussion to #572 would be nice if you can share your input on plan and also if any other open issues are relevant to link.

@danielroe
Copy link
Member Author

danielroe commented Oct 13, 2022

Also, just in case you missed it, this didn't copy files across/mix files from two different versions.

@danielroe danielroe restored the fix/trace-minors branch October 14, 2022 12:48
WinterYukky pushed a commit to WinterYukky/nitro that referenced this pull request Nov 1, 2022
WinterYukky pushed a commit to WinterYukky/nitro that referenced this pull request Nov 1, 2022
This reverts commit d90c938.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants