-
Notifications
You must be signed in to change notification settings - Fork 558
Replacing Lerna with pnpm on LTS #25537
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
Conversation
The build tools version used by LTS isn't compatible with pnpm's workspace:~ dependency protocol, so we had to use an version of build-tools from main which does support that protocol. That introduced a few ancilliary changes we had to make: 1 - build-tools from main requires node>=20. As a result we've updated LTS to use node 20.15.1 (the min required version on main) 2 - Updating to node 20.15.1 required us to update our eslint dependency to a version compatible with node 20 2a - this also introduced a number of linter fixes to various code files 3 - the latest version of build-tools from main had similar but different type test generation code - we had to update the typeValidation JSON in various package.jsons, and the build-tools generated type tests are generated with slightly different names + paths 4 - we disabled the layer-checking policy check on LTS (per offline discussion, we decided it was unlikely there would be major dependency changes to LTS in the future, and so it would be a lot of work for little payoff to enable layer-checking using the latest build-tools version on LTS) 5 - Since main considers azure packages to be part of the client workgroup, we've added azure packages to this change and made them use pnpm 5a - This also lets us remove the build-azure pipeline, since azure gets included with the client packages build 6 - Several pipelines were updated to use pnpm, install pnpm dependencies, and use flub to publish packages. Where possible, the implementations have been pulled directly from main
| "start:server": "node ./dist/server.js", | ||
| "test": "echo \"Error: no test specified\" && exit 1", | ||
| "test:realsvc": "npm run test:realsvc:tinylicious", | ||
| "test:realsvc": "echo Test disabled on LTS", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for reviewers: I disabled this because:
- This actually wasn't running on existing azure pipeline runs (one example: https://dev.azure.com/fluidframework/internal/_build/results?buildId=254938&view=logs&j=3dc8fd7e-4368-5a92-293e-d53cefc8c4b3&t=8ee6b0c5-48a4-51bb-d1f6-a7ad1e52f3d2 )
- The test steps weren't set up right -
test:realsvc:tinylicioustries to invokestart:tinylicious:test, but that script doesn't actually exist, so it fails. I did some best effort work to enable this, but when I got the tests running I found there were too many failures for me to reasonably address in this PR and decided getting it working was out of scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine, scenario-runner is already gone in main and IIRC it had its own pipeline. One could argue maybe we should remove it here but disabling the tests seems fine to me.
| @@ -8,693 +8,698 @@ | |||
| // Auto-generated by policy-check in @fluidframework/build-tools. | |||
|
|
|||
| export const shortCodeMap = { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this addition is right - this is the result of running flub's assertion tagging tool on the repo. I'm surprised it added every assertion message in the repo here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect it, the infrastructure to generate this file during assert tagging came into being once LTS wasn't the active branch. However, as I mentioned in another comment, I'd vote for not running assert tagging if we can avoid it. Not the end of the world if we have to, though (and this file is purely a development-time helper so it's fine if it ends up here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to not running assert tagging. If people care about bundle size, they should bump to modern fluid versions. They can live with string error codes in the meantime if we need to add any asserts to LTS
jason-ha
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been able to cover all root files and directories alphabetically through examples. Look good with some questions.
| "devDependencies": { | ||
| "@fluidframework/build-common": "^1.2.0", | ||
| "@fluidframework/eslint-config-fluid": "^2.0.0", | ||
| "@fluidframework/eslint-config-fluid": "^0.28.2000", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was at 2.0+ and has to go down to 0.28.2000?
(here and other files too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is a bit weird - I made it because eslint-config-fluid has a dependency on eslint-plugin-jsdoc: ~39.3.0, which is incompatible with the new version of node we're using. 0.28.2000 doesn't have that dependency, and since it was being used by other packages I downgraded this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Let's note this and similar work arounds in the PR description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call - added a list of workarounds used to the description
build-tools/package.json
Outdated
| "devDependencies": { | ||
| "@microsoft/api-documenter": "^7.17.9", | ||
| "@microsoft/api-extractor": "^7.22.2", | ||
| "@microsoft/api-extractor": "~7.22.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wish we had comments in package.json. What is this api-extractor ~ for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a workaround for a strange api-extractor issue we were seeing. The problem was when using ^7.22.2, api-extractor was somehow pulling in an incompatible version of typescript to parse the source files and threw errors when parsing. Strangely, even using the --typescript-compiler-folder switch in the run options and setting the version of TS to use wasn't enough. Scott found this workaround of using ~ solved the problem
| }); | ||
|
|
||
| it("There's a button to be clicked", async () => { | ||
| it.skip("There's a button to be clicked", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment?
| }; | ||
|
|
||
| let importedDataView; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine but why any change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I introduced this space while finding a version of eslint + configs that worked under the new config. One config tried making this + subsequent lines a giant ternary statement which ended up violating other rules, so I added // eslint-disable-next-line unicorn/prefer-ternary here, and then later when I had a different config set it didnt try condensing this into a ternary, so the rule was removed but the space remained. I'll remove that space
| "build:ci": "npm run build:genver && lerna run build:compile --stream", | ||
| "build:compile": "lerna run build:compile --stream", | ||
| "build:docs": "lerna run build:docs --stream --parallel", | ||
| "build": "npm run policy-check && npm run build:genver && npm run build:compile && npm run lint && npm run build:docs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe "echo skip layer-check" instead of just removing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if we're skipping layer check, does something else need layerInfo.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good calls. Added that echo and removed layerInfo.json
alexvy86
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went through some of the more mechanical changes, left a few comments. I'd like to still go over the pipeline definitions in more detail when I have time but can't promise I will soon; don't want to be a bottleneck though.
| assert( | ||
| this.mc !== undefined && this.logger !== undefined, | ||
| 0x348, /* this.mc and/or this.logger has not been set */ | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we have tag asserts in this branch? If not necessary I'd recommend reverting this just to avoid introducing functional changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we do - given the added assertionShortCodesMap that got added automatically when I tested, and it picked out a bunch of assert messages when it ran, I think we can undo the output from the assert tagging tool. Will update PR in a bit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take it back - it looks like we did have assert tagging. Most of these messages were pre-existing (turns out everything got highlighted because the linter decided to turn spaces to tabs. Here's the file on LTS prior to my changes: https://github.com/microsoft/FluidFramework/blob/lts/packages/runtime/test-runtime-utils/src/assertionShortCodesMap.ts )
My change did add a couple more assertion tags though, so I'm guessing a few asserts were added in and never got tagged. I also would prefer not to have any functional changes in this PR so I'm still good with undoing the new tags
| "build:ci": "npm run build:genver && lerna run build:compile --stream", | ||
| "build:compile": "lerna run build:compile --stream", | ||
| "build:docs": "lerna run build:docs --stream --parallel", | ||
| "build": "npm run policy-check && npm run build:genver && npm run build:compile && npm run lint && npm run build:docs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if we're skipping layer check, does something else need layerInfo.json?
package.json
Outdated
| "node": ">=18.17.1", | ||
| "npm": "^6.14.18" | ||
| "node": ">=20.15.1", | ||
| "npm": "^6.14.18", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should be removable, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump on this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be, yeah. Removing
Thanks for reviewing! No need to rush - it looks like I missed a couple pipelines that I need to update anyways before this is done |
…cho message, adds comment to disabled test, fixes bad prev version typetest and linter change
| "@microsoft/api-extractor": "~7.34.4", | ||
| "@types/mocha": "^9.1.1", | ||
| "@types/node": "^18.15.3", | ||
| "@types/node": "^14.18.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we have to downgrade @types/node back to 14 at the same time as bumping node to 20?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The types/node change was easily my least favorite change. I tried using a node types file that matched the version of node we're using now. Doing so raised tons of compilation errors in other packages -- which weren't actual errors -- inside the types definitions of that node version. Here's a snippet of the output I get when I change this one package back to using 18.15.3:
packages/common/driver-definitions build:compile: [build:esnext] ~
packages/common/driver-definitions build:compile: [build:esnext]
packages/common/driver-definitions build:compile: [build:esnext] ../../../node_modules/.pnpm/@[email protected]/node_modules/@types/node/http2.d.ts:1177:21 - error TS1005: ',' expected.
packages/common/driver-definitions build:compile: [build:esnext]
packages/common/driver-definitions build:compile: [build:esnext] 1177 listener: (
packages/common/driver-definitions build:compile: [build:esnext] ~
packages/common/driver-definitions build:compile: [build:esnext]
packages/common/driver-definitions build:compile: [build:esnext] ../../../node_modules/.pnpm/@[email protected]/node_modules/@types/node/http2.d.ts:1180:22 - error TS1109: Expression expected.
packages/common/driver-definitions build:compile: [build:esnext]
packages/common/driver-definitions build:compile: [build:esnext] 1180 ) => void,
packages/common/driver-definitions build:compile: [build:esnext] ~
packages/common/driver-definitions build:compile: [build:esnext]
packages/common/driver-definitions build:compile: [build:esnext] ../../../node_modules/.pnpm/@[email protected]/node_modules/@types/node/http2.d.ts:1181:10 - error TS1005: ';' expected.
packages/common/driver-definitions build:compile: [build:esnext]
packages/common/driver-definitions build:compile: [build:esnext] 1181 ): this;
packages/common/driver-definitions build:compile: [build:esnext] ~
packages/common/driver-definitions build:compile: [build:esnext]
packages/common/driver-definitions build:compile: [build:esnext] ../../../node_modules/.pnpm/@[email protected]/node_modules/@types/node/http2.d.ts:1183:18 - error TS1005: ',' expected.
packages/common/driver-definitions build:compile: [build:esnext]
packages/common/driver-definitions build:compile: [build:esnext] 1183 event: "stream",
packages/common/driver-definitions build:compile: [build:esnext] ~
packages/common/driver-definitions build:compile: [build:esnext]
packages/common/driver-definitions build:compile: [build:esnext] ../../../node_modules/.pnpm/@[email protected]/node_modules/@types/node/http2.d.ts:1184:21 - error TS1005: ',' expected.
packages/common/driver-definitions build:compile: [build:esnext]
packages/common/driver-definitions build:compile: [build:esnext] 1184 listener: (stream: ServerHttp2Stream, headers: IncomingHttpHeaders, flags: number) => void,
packages/common/driver-definitions build:compile: [build:esnext] ~
packages/common/driver-definitions build:compile: [build:esnext]
packages/common/driver-definitions build:compile: [build:esnext]
[... continues on...]
The d.ts files are fine, so it shouldnt be throwing errors. I spent a lot of investigation trying to figure this out. I tried to work around it by adding skip lib check options to typescript compilation, but that still didnt work (?!). At this point I looked at main for any inspiration and found main was also using node types from an old version (main's min node is 20.15.1, but most packages use [email protected], and I also spotted this comment in main's package.json: "@types/node is ignored because it is usually not needed by packages, and if it is, then the package will hit a compilation failure.".
At this point I'd been banging my head against a wall on this issue for a couple days and timeboxed it - it looked like main wasn't in an ideal state either, setting everything to node 14.18.0 "worked", so that's how we got here. It's not exactly ideal, I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and note that these errors-which-arent-actually-errors will show up regardless of the version of types/node I use. Trying a more recent types/node for node 20 throws the same stuff:
.../data-objects/multiview/interface build:compile: ../../../../node_modules/.pnpm/@[email protected]/node_modules/@types/node/http2.d.ts:1185:10 - error TS1005: ';' expected.
.../data-objects/multiview/interface build:compile: 1185 ): this;
.../data-objects/multiview/interface build:compile: ~
.../data-objects/multiview/interface build:compile: ../../../../node_modules/.pnpm/@[email protected]/node_modules/@types/node/http2.d.ts:1186:17 - error TS1005: ',' expected.
.../data-objects/multiview/interface build:compile: 1186 on(event: string | symbol, listener: (...args: any[]) => void): this;
.../data-objects/multiview/interface build:compile: ~
.../data-objects/multiview/interface build:compile: ../../../../node_modules/.pnpm/@[email protected]/node_modules/@types/node/http2.d.ts:1186:44 - error TS1005: ',' expected.
.../data-objects/multiview/interface build:compile: 1186 on(event: string | symbol, listener: (...args: any[]) => void): this;
.../data-objects/multiview/interface build:compile: ~
.../data-objects/multiview/interface build:compile: ../../../../node_modules/.pnpm/@[email protected]/node_modules/@types/node/http2.d.ts:1186:70 - error TS1109: Expression expected.
.../data-objects/multiview/interface build:compile: 1186 on(event: string | symbol, listener: (...args: any[]) => void): this;
.../data-objects/multiview/interface build:compile: ~
.../data-objects/multiview/interface build:compile: ../../../../node_modules/.pnpm/@[email protected]/node_modules/@types/node/http2.d.ts:1186:71 - error TS1005: ';' expected.
.../data-objects/multiview/interface build:compile: 1186 on(event: string | symbol, listener: (...args: any[]) => void): this;
.../data-objects/multiview/interface build:compile: ~
| @@ -84,7 +84,7 @@ describeNoCompat("Message size", (getTestObjectProvider) => { | |||
| const containerError = async (container: IContainer) => | |||
| new Promise<IErrorBase | undefined>((resolve) => container.once("closed", (error) => { resolve(error); })); | |||
|
|
|||
| itExpects("A large op will close the container with chunking disabled", [ | |||
| itExpects.skip("A large op will close the container with chunking disabled", [ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment?
| @@ -175,7 +175,7 @@ describeNoCompat("Named root data stores", (getTestObjectProvider) => { | |||
| assert(await dataCorruption); | |||
| }); | |||
|
|
|||
| it("Root datastore creation with aliasing turned on throws exception", async () => { | |||
| it.skip("Root datastore creation with aliasing turned on throws exception", async () => { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And more comments :).
…d tools version, and adding release group tag to test tools pipeline
…me for release group
| "@fluidframework/telemetry-utils-previous": "npm:@fluidframework/[email protected]", | ||
| "@microsoft/api-extractor": "^7.22.2", | ||
| "@fluidframework/mocha-test-setup": "workspace:~", | ||
| "@fluidframework/telemetry-utils-previous": "npm:@fluidframework/[email protected]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous versions for everything should be 1.3.1, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that should be 1.3.1 still. Fixed, thanks!
| "@fluidframework/tool-utils-previous": "npm:@fluidframework/[email protected]", | ||
| "@microsoft/api-extractor": "^7.22.2", | ||
| "@fluidframework/mocha-test-setup": "workspace:~", | ||
| "@fluidframework/tool-utils-previous": "npm:@fluidframework/[email protected]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another previous that I think should be 1.3.1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be. It turns out that there's another issue with this package's typetests - using 1.3.1 introduces this compilation issue in the typetest generated code:
[build:compile] [build:commonjs] dist/odspTokenManager.d.ts:5:23 - error TS2305: Module '"@fluidframework/odsp-doclib-utils"' has no exported member 'IPublicClientConfig'.
[build:compile] [build:commonjs]
[build:compile] [build:commonjs] 5 import { IOdspTokens, IPublicClientConfig } from "@fluidframework/odsp-doclib-utils";
It looks like IPublicClientConfig doesn't exist in 1.3.1, but it's unclear to me why typescript is only now finding a fault with this project when it worked using lerna. I spent a couple hours of best effort trying to resolve the issue with no luck. I've disabled the typetests for tool-utils in the latest update to work around the issue
experimental/PropertyDDS/packages/property-changeset/src/utils.ts
Outdated
Show resolved
Hide resolved
experimental/PropertyDDS/packages/property-properties/src/propertyFactory.js
Outdated
Show resolved
Hide resolved
| (_.isArray(in_obj) !== _.isArray(target)) || | ||
| !_.isObject(target)) { | ||
| throw new Error( | ||
| throw new TypeError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and 236 also go back to Error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed this line :)
…, updates common-utils' build-tools dependency and typetest generation
…st gen for new build tools
…tils typetest to work around unexpected ocmpilation issue
alexvy86
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, a few more comments, and I'm done looking through everything except the pipeline definition files. Since they still seem to need tweaks, I'll wait on those.
| const containerId = await containerCreate.attach(); | ||
| await new Promise<void>((resolve, reject) => { | ||
| await timeoutPromise((resolve, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on the changes to this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the package updates, this test started failing. I strongly suspected the issue was a correctness issue with the test itself - debugging showed it was actually doing the op it was supposed to, but then moved on to assert before everything was settled. When I looked at this test on main (source:
FluidFramework/packages/service-clients/tinylicious-client/src/test/TinyliciousClient.spec.ts
Line 220 in ddaa356
| it("can change initialObjects value", async function () { |
// Make sure the op round tripped
await timeoutPromise((resolve, reject) => {
if (!containerCreate.isDirty) {
resolve();
}
containerCreate.on("saved", () => {
resolve();
});
});
Copying this impl from main resolved the test issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the need for these files? Here, in runtime/test-runtime-utils, and in test-end-to-end-tests (which shouldn't have taggable asserts).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, they shouldnt - the problem was when I ran the assert tagging tool, it tried to tag these test files with asserts. I added these assertTagging.config.mjs files to ensure they would be skipped.
| @@ -28,19 +28,19 @@ | |||
| "test:realsvc": "npm run test:realsvc:local && npm run test:realsvc:tinylicious", | |||
| "test:realsvc:frs": "npm run test:realsvc:run -- --driver=r11s --r11sEndpointName=frs --timeout=20s", | |||
| "test:realsvc:frs:report": "cross-env FLUID_TEST_REPORT=1 npm run test:realsvc:frs --", | |||
| "test:realsvc:local": "npm run test:realsvc:run -- --driver=local", | |||
| "test:realsvc:local": "npm run test:realsvc:run -- --driver=local --baseVersion=1.4.0", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need baseVersion explicitly? Docs suggest that it would default to the current version of the package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should default to the current package version, and actually does when run locally, but the problem I found was these tests didnt use the current version when run on CI. I banged my head against this wall for a good day and a half and ended up using this workaround to resolve the CI failure
| "test:realsvc:local:report": "cross-env FLUID_TEST_REPORT=1 npm run test:realsvc:local --", | ||
| "test:realsvc:odsp": "npm run test:realsvc:run -- --driver=odsp --timeout=20s", | ||
| "test:realsvc:odsp:report": "cross-env FLUID_TEST_REPORT=1 npm run test:realsvc:odsp --", | ||
| "test:realsvc:r11s": "npm run test:realsvc:run -- --driver=r11s --use-openssl-ca", | ||
| "test:realsvc:r11s": "npm run test:realsvc:run -- --driver=r11s --use-openssl-ca --timeout=20s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't mind it but checking if it's strictly necessary to make tests pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not strictly necessary, but useful - I was finding that both CI and local test runs were timing out frequently enough that I added this timeout to reduce noise.
Thank you for the review! The pipeline files are nearly resolved - will ping you once everything's working |
…to what is done on main)
…ks dont work here
Pipelines are now stabilized and ready for review! |
alexvy86
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, done reviewing everything 😄 .
Hopefully last batch of comments. One general one: I didn't look at the lockfile changes under common/, updating package-lock.json from v1 to v3. I'd be slightly concerned about those but one of my comments suggests undoing all the changes under common/ so if we can do that, I think it's probably best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we missed updating the lockfile for this. Although actually, maybe we should consider not updating anything under common/lib/ to minimize churn since those packages are managed separately from the actual FF client code (release group). In main, common/build/ actually matters for the build of the client release group, but in LTS I don't think that's the case? And if so, I think maybe we should skip the changes there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're right, we shouldnt need any changes to the common directory. I've reverted all changes
| (_.isArray(in_obj) !== _.isArray(target)) || | ||
| !_.isObject(target)) { | ||
| throw new Error( | ||
| throw new TypeError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed this line :)
package.json
Outdated
| "node": ">=18.17.1", | ||
| "npm": "^6.14.18" | ||
| "node": ">=20.15.1", | ||
| "npm": "^6.14.18", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump on this one.
| script: | | ||
| echo "installing version @${{ parameters.buildToolsVersionToInstall }}" | ||
|
|
||
| npm install --global "@fluid-tools/build-cli@${{ parameters.buildToolsVersionToInstall }}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this install of build-tools is what needs us to keep the step above that installs npm; I'd argue for removing that step otherwise (like I did in other comments for other yaml files). But if we need it, can we use a consistent version everywhere? npm@10 like the others instead of 6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we do need this. I think we can use a consistent npm version everywhere though, will update existing npm 6s to 10s
| artifactPath: scoped | ||
| feedName: ${{ parameters.feedName }} | ||
| artifactPath: tarballs | ||
| feedKind: ${{ parameters.feedKind }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's very hard to tell if the actual publishing of packages actually works without exercising it; after we merge the PR we'll at least we'll know if publishing to our internal feeds works as expected. At that point maybe doing a "no-op patch" (no-changes) release of the 1.x packages might be a good exercise just to make sure we can actually release them quickly if we ever need to. So nothing to address for this comment, just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I'd like to see the publishing pipelines all working. I think that's a good thing to try out once this is committed. For what its worth, the publish internal test packages step works when I test this on the internal pipelines: https://dev.azure.com/fluidframework/internal/_build/results?buildId=360083&view=logs&j=88a4f9f3-7925-540d-9db1-365e924a72d7 , so I'm crossing my fingers that the'll translate over to the client publish too
…nstalls, removing packageManagerInstallCommand in favor of hardcoded pnpm i
alexvy86
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To the best of my reviewing ability + bandwidth, this looks fine. 🚀
Thank you for reviewing! Going to merge on Monday, just to avoid any potential pipeline failures over the weekend |
This PR replaces lerna for client packages in the LTS branch with pnpm, and merges azure into the 'client' package group
The build tools version used by LTS isn't compatible with pnpm's workspace:~ dependency protocol, so we had to use a version of build-tools from main which does support that protocol. That introduced a few ancilliary changes we had to make:
1 - build-tools from main requires node>=20. As a result we've updated LTS to use node 20.15.1 (the min required version on main)
2 - Updating to node 20.15.1 required us to update our eslint dependency to a version compatible with node 20
2a - Updating eslint also introduced a number of linter changes to various code files
3 - the latest version of build-tools from main had similar but different type test generation code - we had to update the typeValidation JSON in various package.jsons, and the build-tools generated type tests are generated with slightly different names + paths
4 - we disabled the layer-checking policy check on LTS (per offline discussion, we decided it was unlikely there would be major dependency changes to LTS in the future, and so it would be a lot of work for little payoff to enable layer-checking using the latest build-tools version on LTS)
5 - Since main considers azure packages to be part of the client workgroup, we've added azure packages to this change and made them use pnpm
5a - This also lets us remove the build-azure pipeline, since azure gets included with the client packages build
6 - Several pipelines were updated to use pnpm, install pnpm dependencies, and use flub to publish packages. Where possible, the implementations have been pulled directly from main
7 - build-tools sources (and associated pipeline to build build-tools) have been removed. We're now using build-tools from main, which means this code is no longer needed to be built or maintained (NB: the server/ code which uses old build-tools is planned to be removed in a subsequent PR)
Workarounds needed:
api-extractor dependencies are using ~7.22.2 instead of ^7.22.2 now. This was to work around an issue with api-extractor, where ^ dependencies used an inconsistent version of the typescript compiler that caused build failures (and the command line switch to point api-extractor to the right compiler to use did not work)
eslint configurations have been set to use 0.28.2000. This resulted in a downgraded eslint config for some projects. This was done because of the migration to node 20 - eslint config 0.28.2000's dependencies were compatible with node 20, but 2.0.0 included extra dependencies which were incompatible with node 20
@types/node set to 14.18.0 everywhere - trying to use a different types/node version caused typescript compilation failures when processing d.ts files of those libraries, and options like skipLibCheck etc did not fix it. Setting all projects to @types/node 14.18.0 resolved the issue (NB: main has a similar workaround of using node types v 18 due to compilation issues, despite it using node 20+)