-
Notifications
You must be signed in to change notification settings - Fork 236
chore: command orchestration #5791
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
|
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
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.
Would start also require yarn run dev on core?
| "scripts": { | ||
| "build": "yarn workspaces foreach --from '@swc/*' run build", | ||
| "clean": "yarn workspaces foreach --from '@swc/*' run clean", | ||
| "build": "yarn workspaces foreach --from '@swc/*' --recursive run build", |
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 will force Yarn to traverse downstream dependents, rebuilding or cleaning even when nothing changed. Is this what we need? We can do caching or parallelisation here too. Let me know your setup
| "build": "yarn workspaces foreach --from '@swc/*' run build", | ||
| "clean": "yarn workspaces foreach --from '@swc/*' run clean", | ||
| "build": "yarn workspaces foreach --from '@swc/*' --recursive run build", | ||
| "clean": "yarn workspaces foreach --from '@swc/*' --recursive run clean", |
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.
If you just want to delete the SWC output you can avoid the cascading clean. You can just run this?
yarn workspaces foreach --from '@swc/*' run clean
package.json
Outdated
| "build-first-gen": "yarn workspace @adobe/spectrum-web-components build", | ||
| "build-second-gen": "yarn workspace @adobe/swc build", | ||
| "build": "yarn build-first-gen && yarn build-second-gen", |
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 strictly a curiosity I had: can you explain why you chose build-first-gen over something like build:first-gen? I've just seen the colon syntax more often, that's all.
Sort of along these lines, if we went with the colon syntax instead, could we do something like:
"build": "yarn build:*",
Why does the order of building first-gen or second-gen matter? I honestly don't know what order something like yarn build:* would build each project, so maybe this is not even a good idea or feasible! 😆
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.
For the first part, It was an oversight on my end. I feel its better to use build:first-gen and so I switched that up. And about the second one, no the order of building first-gen or second-gen doesn't matter as long as both are being built before starting their respective storybooks.
second-gen/packages/swc/package.json
Outdated
| "prestorybook": "cem analyze", | ||
| "storybook": "storybook dev -p 6006", | ||
| "storybook:build": "storybook build", | ||
| "test": "yarn workspace @swc/core dev & vitest", |
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 seems like "test:watch", is that accurate?
ff96761 to
85e914c
Compare
.eslintrc.json
Outdated
| ], | ||
| "parser": "@typescript-eslint/parser", | ||
| "parserOptions": { | ||
| "ecmaVersion": 2020, |
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 we just use "latest" here or is there a reason we need to use "2020" version?
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.
no reason. it was 2020 everywhere else so cursor copied the same thing and i didn't look. will update this to latest and see if nothing breaks
| "lint:first-gen": "yarn workspace @adobe/spectrum-web-components lint", | ||
| "lint:second-gen": "yarn workspace @adobe/swc lint", | ||
| "postinstall": "husky || true && patch-package", | ||
| "start": "yarn start:first-gen & yarn start:second-gen", |
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.
for some reason prestorybook hook was not getting called and thus this error.
i chained prestorybook explicitly to run before storybook in second-gen packages and it's working now
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 was still getting the same error associated with storybook setup not having dirname correctly defined since we are using modules. i have implemented a fix and will push it up with my review.
| "lint": "yarn lint:first-gen && yarn lint:second-gen", | ||
| "lint:first-gen": "yarn workspace @adobe/spectrum-web-components lint", | ||
| "lint:second-gen": "yarn workspace @adobe/swc lint", | ||
| "postinstall": "husky || true && patch-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.
why do we need patch-package? we can patch packages with yarn4 now and have removed patch-package from first gen to my knowledge
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.
great question but this is out of scope for the current PR. will track it on slack maybe?
abce0f2 to
1b9f9d0
Compare
| "prestorybook": "cem analyze", | ||
| "storybook": "storybook dev -p 6006", | ||
| "storybook:build": "storybook build", | ||
| "storybook": "yarn analyze && storybook dev -p 6006", |
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.
yarn cem:watch && storybook dev -p 6006?
second-gen/packages/swc/package.json
Outdated
| "scripts": { | ||
| "analyze": "cem analyze --config cem.config.js", | ||
| "build": "vite build", | ||
| "cem:watch": "cem analyze --watch", |
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.
nit, for consistency, could we do analyze:watch?
|
There’s a small issue: if Storybook for the first-generation package hasn’t been built yet, it won’t start successfully. Running ERROR in ./storybook/preview.js 13:0-90
Module not found: Error: Can't resolve '@spectrum-web-components/story-decorator/decorator.js' in '/Users/ruben/Documents/Work/swc/first-gen/storybook'
@ ./storybook-config-entry.js 10:1309-1382 40:2-43:4 40:1280-43:3
ERROR in ./storybook/preview.js 14:0-82
Module not found: Error: Can't resolve '@spectrum-web-components/story-decorator/src/locales.js' in '/Users/ruben/Documents/Work/swc/first-gen/storybook'
@ ./storybook-config-entry.js 10:1309-1382 40:2-43:4 40:1280-43:3
ERROR in ./storybook/preview.js 17:0-72
Module not found: Error: Can't resolve '@spectrum-web-components/story-decorator/sp-story-decorator.js' in '/Users/ruben/Documents/Work/swc/first-gen/storybook'
@ ./storybook-config-entry.js 10:1309-1382 40:2-43:4 40:1280-43:3
preview compiled with 3 errors
=> Failed to build the preview
<s> [webpack.Progress] 99% end closing watch compilation
<s> [webpack.Progress] 99% end closing watch compilation
WARN Force closed preview build
SB_BUILDER-WEBPACK5_0003 (WebpackCompilationError): There were problems when compiling your code with Webpack.
Run Storybook with --debug-webpack for more information.
at starter (/Users/ruben/Documents/Work/swc/node_modules/@storybook/builder-webpack5/dist/index.js:1:24163)
at starter.next (<anonymous>)
at Module.start (/Users/ruben/Documents/Work/swc/node_modules/@storybook/builder-webpack5/dist/index.js:1:26142)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async storybookDevServer (/Users/ruben/Documents/Work/swc/node_modules/@storybook/core/dist/core-server/index.cjs:36423:79)
at async buildOrThrow (/Users/ruben/Documents/Work/swc/node_modules/@storybook/core/dist/core-server/index.cjs:35049:12)
at async buildDevStandalone (/Users/ruben/Documents/Work/swc/node_modules/@storybook/core/dist/core-server/index.cjs:37628:78)
at async withTelemetry (/Users/ruben/Documents/Work/swc/node_modules/@storybook/core/dist/core-server/index.cjs:35788:12)
at async dev (/Users/ruben/Documents/Work/swc/node_modules/@storybook/core/dist/cli/bin/index.cjs:5874:3)
at async s.<anonymous> (/Users/ruben/Documents/Work/swc/node_modules/@storybook/core/dist/cli/bin/index.cjs:6052:74)
⠇
WARN Broken build, fix the error above.
You may need to refresh the browser. |
second-gen/.eslintrc.json
Outdated
| "lit-a11y/click-events-have-key-events": [ | ||
| "error", | ||
| { | ||
| "allowList": [ |
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 are we including this in second gen? this is very much a first-gen pattern.
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 necessarily a part of this pr bt good catch. I shall remove 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.
Can we make sure to review the linter settings we want in second-gen to ensure they are patterns we want to continue using or add new ones? This is not blocking for this PR but want to make sure we discuss as a team
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.
What currently is here is just a simple setup to get eslinting up and running. We can and should talk about it :)
| } | ||
| ], | ||
| "no-debugger": 2, | ||
| "notice/notice": [ |
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.
we need to make sure this is still working in second gen, we should still be prepending a copywright to every 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.
Why was this removed?
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.
Does the root level work across the project?
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 removed it cause we don't have a license.js config for second-gen and i don't know if we want to replicate the first-gen or not?
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.
We do, yes. License should be project-wide
6f57fd4 to
29f4e28
Compare
rubencarvalho
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.
gut gut!!
caseyisonit
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.
crushed it







Description
Motivation and context
Related issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Build Pipeline (First-gen and Second-gen)
Test Scripts
Linting
Wireit Integration
CI/CD
Device review