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

Update @oclif/core to 1.21.0 to fix the dependency installation issue in the fixture app #879

Merged
merged 5 commits into from Dec 15, 2022

Conversation

pepicrft
Copy link
Contributor

@pepicrft pepicrft commented Dec 8, 2022

WHY are these changes introduced?

dev and build install dependencies by default. When they do, they use the lockfile in the project directory to determine the package manager to use. Because the fixture project is part of the repository workspace, there's no PNPM lockfile and therefore tries to install dependencies there with npm leading to the following error reported by @amcaplan:

  82773 ── external error ─────────────────────────────────────────────────────────────────
  82774 
  82775 Error coming from `npm install`
  82776 
  82777 Command failed with exit code 1: npm install
  82778 npm ERR! Cannot read properties of null (reading 'matches')
  82779 
  82780 npm ERR! A complete log of this run can be found in:
  82781 npm ERR!     /Users/amcaplan/.npm/_logs/2022-12-08T12_00_24_674Z-debug-0.log
  82782 
  82783 ───────────────────────────────────────────────────────────────────────────────────

WHAT is this pull request doing?

Until we can have a discussion around the responsibilities of the CLI and the future of the --skip-dependencies-install flag, I'm configuring dev.yml to set the environment SHOPIFY_FLAG_SKIP_DEPENDENCIES_INSTALLATION: 1 and disable that functionality while working in the monorepo because it's not necessary.
Setting the variable is not sufficient because the version of @oclif/core that we are using only uses the environment variables for flags that represent options. Therefore I'm updating the version of @oclif/core to have @amcaplan's fix oclif/core#536.

How to test your changes?

Try to create a new app and interact with it. Also try the commands against the main fixture app.

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

@pepicrft pepicrft self-assigned this Dec 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 8, 2022

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

  • Themes: @shopify/theme-developer-tools
  • UI extensions: @shopify/ui-extensions-cli
  • Hydrogen: @shopify/hydrogen
  • Other: @shopify/cli-foundations

@amcaplan
Copy link
Contributor

amcaplan commented Dec 8, 2022

Note that in the past we've explicitly reverted a version upgrade due to concerns about breaking changes in oclif (which prompted my PR to oclif that you mentioned). I think we'd discussed adding some high-level tests (likely some kind of golden master) to gain more confidence that the breaking changes haven't broken anything.

Is it a good idea to change that approach now?

@pepicrft
Copy link
Contributor Author

pepicrft commented Dec 9, 2022

Is it a good idea to change that approach now?

Breaking changes can and will happen in the packages we depend on. I'd say that we trust them on this one and gain confidence through some manual testing, and if it turns out breaking-changes in minor versions becomes a norm in @oclif, then we can be more defensive through additional tests on our end. What do you think?

@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 67.61% 3479/5146
🟡 Branches 61.86% 1108/1791
🟡 Functions 65.32% 870/1332
🟡 Lines 67.77% 3312/4887

Test suite run success

863 tests passing in 435 suites.

Report generated by 🧪jest coverage report action from 9d46583

@pepicrft pepicrft force-pushed the oclif-upgrade branch 3 times, most recently from ade8e9b to d0804c2 Compare December 14, 2022 16:07
Copy link
Contributor

@amcaplan amcaplan left a comment

Choose a reason for hiding this comment

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

Works on my machine! 👩‍💻

I'm a bit hesitant to say "YOLO" given we've been burned on this in the past... but I also see that most of their commits are dependabot upgrades anyway, and hopefully we're tophatting enough that we'll notice any new bugs.

I would like to see if we can have some kind of golden master testing for future reference. Even if nothing is really broken, small things can change in these upgrades and it would be good at least to be made aware of those changes.

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