Skip to content

Conversation

@NSeydoux
Copy link
Contributor

@NSeydoux NSeydoux commented Dec 9, 2022

No description provided.

This removes using rollup node-resolve plugin and typescript project references, all that was needed was to set the build order in the workspaces field for the compilation to suceed.
@NSeydoux NSeydoux marked this pull request as ready for review December 12, 2022 10:32
@NSeydoux NSeydoux requested a review from a team as a code owner December 12, 2022 10:32
Copy link
Contributor

@ThisIsMissEm ThisIsMissEm left a comment

Choose a reason for hiding this comment

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

I'd like to better understand why this is failing often.

package.json Outdated
"packages/internal-test-env",
"packages/internal-playwright-testids",
"packages/internal-playwright-helpers",
"packages/jest-jsdom-polyfills"
Copy link
Contributor

Choose a reason for hiding this comment

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

This change shouldn't be needed, this just tells npm to glob that directory..

Copy link
Contributor Author

@NSeydoux NSeydoux Dec 12, 2022

Choose a reason for hiding this comment

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

I'll try what you suggested in your comment and have lerna run the build, because I actually thought that was already the case and that lerna was hooking into the workspaces setup. This changes just adds an order in which the packages are built, because using alphabetical order results in packages/internal-playwright-helpers being built first, while it has dependencies on the other packages.

@ThisIsMissEm
Copy link
Contributor

If you're seeing npm run build --workspaces --if-present fail due to dependencies between packages, then the answer will be to use lerna to run the build instead, as npm workspaces are topologically sorted, iirc.

@ThisIsMissEm
Copy link
Contributor

Aside, it can't hurt to add a ci.yml which always runs a build? Might be necessary now that we're getting more complex packages here

Building based on workspaces alone (i.e. `npm run build --workspaces`) doesn't account for dependencies between workspaces, resulting in an arbitrary build order being based on the globbing pattern/order in which workspaces are defined. Instead, this commit uses Lerna to run the build, and relies on Nx to figure out the build order based on the dependency graph.
@@ -1,5 +1,5 @@
{
"useNx": false,
"useNx": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Reading this page, it all makes sense the order of operations being taken care of by lerna here with help from Nx to see it that it cannot build the helpers package before testids. https://nx.dev/recipes/adopting-nx/lerna-and-nx Makes sense 👍

@NSeydoux NSeydoux merged commit c2df56d into main Dec 12, 2022
@NSeydoux NSeydoux deleted the chore/fix-workspace-build-again branch December 12, 2022 15:08
# the other way around results in top-level dependencies (namely, lerna)
# not being installed.
- run: npm ci
- run: npm run build
Copy link
Contributor

Choose a reason for hiding this comment

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

Release runs build as part of it, or, at least should. Part of publishing a package is running the build step iirc

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.

4 participants