-
Notifications
You must be signed in to change notification settings - Fork 6
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
#40: Add turbo and pnpm syncing support #119
Conversation
|
Preview URLsEnv: preview |
1a4d1eb
to
ea8cb93
Compare
ea8cb93
to
87acf26
Compare
TypecheckDocs: | ||
name: Typecheck Docs | ||
runs-on: ubuntu-latest | ||
needs: build | ||
steps: | ||
- uses: actions/checkout@v3 | ||
with: | ||
persist-credentials: false | ||
- uses: ./.github/actions/pnpm | ||
- uses: ./.github/actions/download-built-package | ||
- run: pnpm glint | ||
working-directory: docs-app |
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 now accomplished with the lint
step above instead of being in a separate step 🎉
- uses: marocchino/sticky-pull-request-comment@v2 | ||
with: | ||
message: |+ | ||
## Preview URLs | ||
GH Env: ${{ needs.PublishDocstoCloudflarePages.outputs.env }} | ||
docs: ${{ needs.PublishDocstoCloudflarePages.outputs.url }} | ||
# api docs: ${{ needs.PublishDocstoCloudflarePages.outputs.url }}/api/modules.html |
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 ahead and deleted this comment as we aren't doing these yet (but also no plan to soon™).
49c905c
to
0991fef
Compare
0991fef
to
f925416
Compare
For local development, do we still have to set up
Without that I am still getting this error... I do notice that the |
"start": "ember serve", | ||
"test:ember": "ember test" | ||
"test:ember": "ember test", | ||
"_syncPnpm": "pnpm sync-pnpm" |
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.
Oh do I have to run this script locally?
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.
Nope! You can still use pnpm linking locally as it works today!
@nicolechung Yup! We will still need to do this for now
Interesting. Can you provide some reproduction steps? I tried the following with success: rm -rf node_modules
pnpm I
# a build is still required for the docs-app/test-app to consume core!
cd packages/ember-toucan-core
pnpm build
cd ../../docs-app/
pnpm start and then from the root: pnpm start:docs I wonder if your node_modules were in a weird state or something? Based on the error message I'd also guess that maybe there's not a build for May also try |
I did a full But I didn't do |
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.
🎉
🚀 Description
This PR adds
turbo
andpnpm
syncing to matchember-headless-form
. This PR should align with what is in CrowdStrike/ember-headless-form#52.This cuts CI time by ~2mins, or in half!
Closes #40
🔬 How to Test
No visual changes, all CI related
📸 Images/Videos of Functionality
N/A