chore: add basic project constraints #5152
Conversation
|
Branch previewReview the following VRT differencesWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
Lighthouse scores
What is this?Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on Transfer Size
Request Count
|
Pull Request Test Coverage Report for Build 14064918594Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
yarn.config.cjs
Outdated
| workspace.set('main', './src/index.js'); | ||
| workspace.set('module', './src/index.js'); |
There was a problem hiding this comment.
Some of our packages do not have a different entry point other than index.js (e.g. clear-button or opacity-checkerboard), so we might need to account for these exceptions.
There was a problem hiding this comment.
There's a couple different ways to handle it, would love your thoughts:
- We wrap the set command in a check first so it only adds the value if it's not already set. This prevents overwriting a value however it also negates the benefit of being able to manage the consistency of all packages.
- We add an exclusion for those packages that we know have exceptions (ideal if those exceptions are consistent to each other).
There was a problem hiding this comment.
Good call! I think having an explicit list of exceptions (like clear-button and opacity-checkerboard etc) makes sense while keeping everything else consistent. That way, we don’t override anything we shouldn’t. What do you think?
There was a problem hiding this comment.
@rubencarvalho Still looking for your feedback when you have a moment. Thanks!
There was a problem hiding this comment.
I haven't heard back so I went ahead and implemented option 2 above.
There was a problem hiding this comment.
Oops, sorry - this slipped under my radar. Yes, I would’ve voted for option 2, so thank you for that!
castastrophe
force-pushed
the
castastrophe/chore-add-constraints-settings
branch
2 times, most recently
from
31281f7 to
eb1f4d4
Compare
| * Fetch a list of all the component workspaces using a glob pattern | ||
| * @type {string[]} components | ||
| */ | ||
| const components = fg.sync('packages/*', { |
There was a problem hiding this comment.
fg.sync() is called synchronously here, which might block execution since there are a lot of packages.
can we use an async function and await fg()
There was a problem hiding this comment.
There's no difference here between using await or sync because the list of components must be present to validate against in the functions below. This is a preference rather than an important requirement to update during code review. There is the same amount of "blocking execution" as you say, between awaiting an async function as there is using the synchronous version of a function. You only gain benefits from asynchronous if you are running tasks against it as the information becomes available, which we are not.
| const localPackages = Object.keys( | ||
| workspace.manifest.peerDependencies | ||
| ) | ||
| .filter((pkg) => Yarn.workspace({ ident: pkg })) |
There was a problem hiding this comment.
Yarn.workspace({ ident: pkg }) is called twice on a single package here. can we reuse the result and call it once?
const localPackages = Object.keys(workspace.manifest.peerDependencies)
.map((pkg) => Yarn.workspace({ ident: pkg }))
.filter(Boolean);There was a problem hiding this comment.
See my note on using the standard constraints syntax and tooling. This is not a node script. https://yarnpkg.com/features/constraints
There was a problem hiding this comment.
Yarn.workspace({ ident: pkg }) returns a filtered result based on the object keys specific to the workspace passed into the function. You can't call it "once" as you say and get the same result.
| * Process the components workspaces with component-specific configuration | ||
| */ | ||
| if (isComponent) { | ||
| const folderName = workspace.cwd?.split('/')?.[1]; |
There was a problem hiding this comment.
A more robust approach like this path.basename(workspace.cwd) would be helpful?
There was a problem hiding this comment.
It's important to distinguish the contraints syntax from a node script. This is not a node script. This is a yarn constraints file that uses node syntax. Please do read up on constraints in the documentation: https://yarnpkg.com/features/constraints
| 'design-system', | ||
| 'spectrum', | ||
| 'adobe', | ||
| 'adobe-spectrum', | ||
| 'web components', | ||
| 'web-components', | ||
| 'lit-element', | ||
| 'lit-html', |
There was a problem hiding this comment.
Can we add the values to a new array like baseKeyWords and use this? Saves re-allocation of new array everytime keywords is called!
There was a problem hiding this comment.
Minimally beneficial and I'm not sure the abstraction is worth it. If the script was slow to run, I'd say the optimization makes sense but I'm not sure it's worth more abstraction in an already foreign syntax to most contributors.
castastrophe
force-pushed
the
castastrophe/chore-add-constraints-settings
branch
2 times, most recently
from
2b8029d to
6377218
Compare
castastrophe
force-pushed
the
castastrophe/chore-add-constraints-settings
branch
3 times, most recently
from
56e59f7 to
4c0f84b
Compare
castastrophe
requested review from
Rajdeepc and
rubencarvalho
and removed request for
Rajdeepc
castastrophe
force-pushed
the
castastrophe/chore-add-constraints-settings
branch
2 times, most recently
from
1c13a2e to
14338ec
Compare
Co-authored-by: Rúben Carvalho <[email protected]>
castastrophe
force-pushed
the
castastrophe/chore-add-constraints-settings
branch
from
538b727 to
1880c55
Compare
Description
Yarn constraints allow enforcement of rules across workspaces in a project.
This update does not apply the constraints, but rather defines them so they can be reported. We can do a separate PR to apply the updates via:
yarn constraints --fix.Motivation and context
This update adds package.json standardization rules such as:
For non-component packages, this update expects packages:
Component-specific packages expect:
How has this been tested?
yarn constraints: expect it to report the following proposed fixesTypes of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.