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

chore(deps): update semver ranges for @stencil/core package #318

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

alicewriteswrongs
Copy link
Member

@alicewriteswrongs alicewriteswrongs commented Jan 5, 2023

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally for affected output targets
  • Tests (npm test) were run locally and passed
  • Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue URL:

What is the new behavior?

This change updates the semver ranges for the @stencil/core peer dependency for various framework wrapper packages. This should let us use them with stencil v3.

Does this introduce a breaking change?

  • Yes
  • No

testing

I think this all works, but to be honest I am not the biggest expert on this repo.

Here's what I did to check that it's working.

building output targets

First we need to build and package test versions of the framework wrapper packages which have the new version.

This will bootstrap the repo, run build in all the packages, and then run npm pack and npm link in each of them (I'm not super familiar with lerna so very probably there is a better way to do this!):

npm run bootstrap
npm run build
npx lerna exec npm pack
npx lerna exec npm link

After that we should have a .tgz file for each built and set up so that we can install it on the framework side.

install in framework

Ok so basically we'll need to install the packages we just built in framework and install Stencil v3.

First we need to link the framework wrapper packages we just built. Clone the framework repo and in the root of it do the following:

cd core
npm i
npm link @stencil/angular-output-target --save
npm link @stencil/react-output-target --save
npm link @stencil/vue-output-target --save

You may not need the --save flag - I ran into a confusing interaction between npm, lerna, and volta and this was the way that I got it to work 🤷‍♀️

Once you have the framework wrapper packages installed then you'll want to install the v3 beta of Stencil, like so:

npm i --save @stencil/core@3.0.0-beta.0 --force

You should still see some peer dependency errors, but they should only be from @stencil/sass and not from @stencil/angular-output-target and company.

Ok finally we can run a build in Framework:

npm run build

build and test react, angular, and vue packages in framework

Now we need to test that we can build and test the framework-specific packages for the Ionic framework. We can lean on Lerna for a bunch of this.

First I had to do this to get package dependencies to install correctly:

npx lerna exec "npm install --legacy-peer-deps"

then we can build all the packages by doing

npx lerna run sync
npx lerna run build --ignore @ionic/docs

🚨⚠️NOTE⚠️🚨 at present the build does not work because of a known issue with circular type exports. However, repeating the above steps with @stencil/core@2.18.0 installed does work.

@alicewriteswrongs alicewriteswrongs marked this pull request as ready for review January 5, 2023 20:45
@alicewriteswrongs alicewriteswrongs requested review from a team as code owners January 5, 2023 20:45
@@ -15,7 +15,7 @@
},
"devDependencies": {
"@ionic/prettier-config": "^2.0.0",
"@stencil/core": "^2.9.0",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think NPM knows how to bump packages with text in their version string. e.g. I'm not sure if it knows that v3.0.0-rc.0 (the next type of release we'll do) is a higher version than v3.0.0-beta.0. But maybe I'm wrong there - do you know if that's the case?

Ideally, I don't want to have to put up a PR when we go from beta->rc->ga, mostly so we don't have as tight of a coordination schedule when we're rolling v3.0.0 out. In the event that NPM doesn't know that rc comes after beta (and that 3.0.0 comes after rc), I'm not sure what the best course of action is here. IIRC npm can get finicky here. Maybe using v3-next in our accepted versions string in addition to ^3.0.0? I'm not 100% sure npm will accept that without doing something like:

  1. Installing v3-next with npm i @stencil/core@v3-next
  2. Manually edit the dependencies
  3. Installing the latest Stencil Core (on v2) with npm i @stencil/core@latest)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I'm not 100% sure on some of these range matching bits, but I went ahead with this range after checking here: https://jubianchi.github.io/semver-check/#/^3.0.0-beta.0/3.0.0

and doing some reading of the semver package documentation, here: https://github.com/npm/node-semver

I believe that, for instance, 3.0.0 or 3.0.1 will be accepted as meeting ^3.0.0-beta.0 (as will, I think, 3.0.0-beta.1, 3.0.0-rc.0, etc).

@@ -15,7 +15,7 @@
},
"devDependencies": {
"@ionic/prettier-config": "^2.0.0",
"@stencil/core": "^2.9.0",
Copy link
Member

Choose a reason for hiding this comment

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

Should we update the package-lock.json files here (and in the other files) as well? devDependencies and peerDependencies still get written to the manifest when running npm i. I think this might get a little tricky with respect to my other comment on this line though 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yeah that's probably a good idea 😅

I think I missed it just because I was lerna-ing and I thought it would do that automatically!

Copy link
Member

@rwaskiewicz rwaskiewicz left a comment

Choose a reason for hiding this comment

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

2 Things:

  1. I had to update Stencil Sass as well when testing this. I didn't think we'd need to update that, but I was wrong it appears. I'll put up the PR in a bit to fix that. (chore(deps): support stencil v3 stencil-sass#198)
  2. For the example projects, it looks like there's a lot of changes in the component-library package-lock, some in the vue one, and none for angular/react. Do we need any of them? Do we need to update Angular & React (or were they up to date?)?

@alicewriteswrongs
Copy link
Member Author

For the package-lock.json I'm not sure, I just ran an npm install in each package and then committed what changed

@rwaskiewicz
Copy link
Member

Ah, I suspect that these haven't been updated in that case :-(

Well, I suppose we need to update at some point, might as well do it now 🚢

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