-
Notifications
You must be signed in to change notification settings - Fork 35
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
Move sku to a package within the monorepo #754
Conversation
|
@@ -4,5 +4,6 @@ | |||
"commit": false, | |||
"linked": [], | |||
"access": "public", | |||
"ignore": ["@sku-fixtures/*", "@sku-private/*"], |
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 make sure the re-location didn't cause issues with the built package I tested a snapshot in a few SEEK apps and they all worked fine. |
packages/sku/README.md
Outdated
|
||
## Contributing | ||
|
||
Refer to [CONTRIBUTING.md](./CONTRIBUTING.md). If you're planning to change the public API, please [open a new issue](https://github.com/seek-oss/sku/issues/new) and follow the provided RFC template in the [GitHub issue template](.github/ISSUE_TEMPLATE.md). |
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 relative path likely needs to be changed.
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.
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.
Is it worth a snapshot just to make sure it works well on npm?
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.
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.
Pushed to a separate branch to avoid having to clean up a test changeset in this branch.
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.
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.
README links aren't correct. (Please dismiss once changed)
Currently the
sku
package is also the monorepo root. This is generally a weird pattern as far as monorepos go. In addition, pnpm's symlinkednode_modules
causes an infinite glob loop when validating peer dependencies in fixtures (because fixtures link to sku which contains fixtures which link to sku...). This infinite glob loop was the reason we had to setSKU_FORCE_EXIT=true
when running tests, because the validation is intentionally not awaited.This doesn't really cause any issues on an M1 Pro macbook, but the build agents, with their measly 2 cores, don't like it at all. A max depth can be set on this glob, but I'm not sure what a good value for the depth is, as nested
node_modules
are valid, to a certain extent. Ultimately, fixing the monorepo structure by movingsku
to its own package within the repo is a better solution, and that's what this PR does.Consequently, both local and CI tests have gotten significantly faster (about 33% faster on my macbook, probably more on CI (and even more if we get a jest cache hit!)), both due to the structural change and the fact that we can now set
--maxWorkers=2
on CI when running tests.The primary changes in this PR are:
packages/sku
(this is the bulk of the changes)package.json