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(deps): fixed yargs import for yarn 3.x + pnp #2348

Merged
merged 1 commit into from
Nov 19, 2021
Merged

fix(deps): fixed yargs import for yarn 3.x + pnp #2348

merged 1 commit into from
Nov 19, 2021

Conversation

leaumar
Copy link
Contributor

@leaumar leaumar commented Nov 18, 2021

fixes #2332 , process of arriving at this change documented there

I did not make this change out of some sort of expertise in js dependency loading mechanisms, but by trial and error and eliminating the odd one out. I did however test this change in my personal project by modifying the downloaded npm artifact's code in the same way, and I ran npm test in this repo after this change too. If I understand the author's comment right, this change can't be harmful.

I'm willing to put in more time to work with you on proving this change fixes something, but please reach out with instructions then. I'll give you a live demo via a discord call if you want, or a repo to reproduce it on (which is already linked in the issue).

@leaumar leaumar changed the title Use alt yargs import so web-ext works in yarn+pnp Use alt yargs import so web-ext works in yarn+pnp 3.x Nov 18, 2021
@leaumar leaumar changed the title Use alt yargs import so web-ext works in yarn+pnp 3.x fix(deps): fixed yargs import for yarn 3.x + pnp Nov 18, 2021
@codecov
Copy link

codecov bot commented Nov 18, 2021

Codecov Report

Merging #2348 (13c0446) into master (19fdd3a) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2348   +/-   ##
=======================================
  Coverage   99.88%   99.88%           
=======================================
  Files          32       32           
  Lines        1699     1699           
=======================================
  Hits         1697     1697           
  Misses          2        2           
Impacted Files Coverage Δ
src/program.js 100.00% <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 19fdd3a...13c0446. Read the comment docs.

Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

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

@leaumar lgtm, it looks that we can now also remove that $FlowFixMe and related todo comments, I also confirmed locally that the flowtype checks are passing just fine without that now. Would you mind to remove line 9 and 10 in this PR? (if you can't, don't worry, I can take care of that in a separate patch).

As I have also expressed in the issue comments, thank you for having looked into how to fix this.

@leaumar
Copy link
Contributor Author

leaumar commented Nov 19, 2021

I'll push that in a minute, no prob.

I feel we should forward this issue to the yargs devs too, since the cause is an inconsistency in their exports and there may be other middle projects breaking on this. I'll just go ahead and refer them here?

@leaumar
Copy link
Contributor Author

leaumar commented Nov 19, 2021

Do the current changes on master since the last release permit a speedy patch release btw? Would love this to be rolled out asap if possible.

@rpl
Copy link
Member

rpl commented Nov 19, 2021

Do the current changes on master since the last release permit a speedy patch release btw? Would love this to be rolled out asap if possible.

I was already planning to release a minor version soon enough (I've been a bit busy, but I should be able to get to it at least by next week) and merging on master PRs that fit in what I consider ok for a minor version. This applies too.

@rpl
Copy link
Member

rpl commented Nov 19, 2021

I'll push that in a minute, no prob.

I feel we should forward this issue to the yargs devs too, since the cause is an inconsistency in their exports and there may be other middle projects breaking on this. I'll just go ahead and refer them here?

sure, feel free to go ahead and report it to yargs, it is possible that there may be reasons that we may be missing or to be something that would be at least worth to document more explicitly in the yargs docs or README, either case yargs maintainers will know if they feel that is something they want to follow up to on their side or not.

@rpl rpl merged commit 99ccb73 into mozilla:master Nov 19, 2021
@leaumar
Copy link
Contributor Author

leaumar commented Nov 19, 2021

Alright, thanks. Is gonna feel good to stop waking up to tons of CI failure emails every morning after Renovate rebases all the failing dependency bump PRs. 😛

@leaumar leaumar deleted the patch-1 branch November 19, 2021 16:21
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.

Breaks under yarn >=3.0.0
2 participants