-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
docs: finalise docs-snippets
#3369
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…fuels-ts into ps/docs/finalise-docs-snippets
CodSpeed Performance ReportMerging #3369 will not alter performanceComparing Summary
|
We need to make some changes to the I'm opening this for review as it should be isolated to the |
…nalise-docs-snippets
…fuels-ts into ps/docs/finalise-docs-snippets
Coverage Report:
Changed Files:
|
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.
Awesome job on this @petertonysmith94 , so happy to finally put these doc changes to bed 🚀 😃
I've left some comments and questions.
apps/docs/.spellcheck.yml
Outdated
context_visible_first: true | ||
escapes: \\[\\`~] | ||
delimiters: | ||
- name: SPCheck |
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 looks like just a formatting change
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.
apps/docs/README.md
Outdated
pnpm build | ||
``` | ||
|
||
The snippets are built into testable scripts from the `src/snippets` folder and generates the Typegen types into the `src/snippets/typegend` folder. All test scripts end with a `.test.ts` suffix. |
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.
The snippets are built into testable scripts from the `src/snippets` folder and generates the Typegen types into the `src/snippets/typegend` folder. All test scripts end with a `.test.ts` suffix. | |
The build process generates testable scripts from the snippets and outputs them alongside the snippets. The corresponding Typegen types are also generated and outputted in the `src/snippets/typegend` folder. All test scripts end with a `.test.ts` suffix. |
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.
apps/docs/README.md
Outdated
|
||
## Testing | ||
|
||
This will build the snippets and run the generated tests. To test a specific environment (`node` or `browser`), the snippet should be named as `{name}.{environment}.test.ts`. e.g. `deploy-contract.node.test.ts` |
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 had discussed this before on a previous PR but essentially @danielbate made the point about us standardizing how we want to specify test environments by continuing to use the group comments as opposed to using file suffixes. I think we should perhaps continue in that vain.
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.
apps/docs/scripts/pretest.sh
Outdated
@@ -1,5 +1,5 @@ | |||
# Check if node is already running at port 4000, if not start it | |||
# TODO: This is a temporary solution to avoid conflicts with the test node in docs-snippets2 | |||
# TODO: This is a temporary solution to avoid conflicts with the test node in docs-snippets |
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 should no longer be necessary, perhaps we can revert this to 8e80ba1
# Kill anything running on port 4000
lsof -t -i:4000 | xargs -r kill
# Runs a node at port 4000
pnpm fuels node > /dev/null 2>&1 &
# Builds projects
pnpm fuels build
# Deploys projects (needed for loader bytecode)
pnpm fuels deploy
# Kills the node
lsof -t -i:4000 | xargs -r kill
# Checks for type errors
pnpm tsc --noEmit
Although I don't think the type check is necessary anymore post-#3381
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.
apps/docs/scripts/wrap-snippets.ts
Outdated
`src/vite.config.ts`, | ||
`src/versions.data.ts`, | ||
`src/jsonc.d.ts`, | ||
`src/guide/transactions/new-api.ts`, |
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 introduced on this PR but does this file exist? Not seeing it
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.
"prettier:check": "prettier --check packages --check apps/docs-snippets", | ||
"prettier:format": "prettier --write packages --check apps/docs-snippets", | ||
"prettier:check": "prettier --check packages --check apps/docs", | ||
"prettier:format": "prettier --write packages --write apps/docs", |
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 this --write
intentional?
"prettier:format": "prettier --write packages --write apps/docs", | |
"prettier:format": "prettier --write packages --check apps/docs", |
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.
Yes, I'd expect to fix the changes, as it's the prettier:format
command.
@@ -1,8 +1,10 @@ | |||
{ | |||
"extends": "../../tsconfig.base.json", | |||
"compilerOptions": { | |||
"outDir": "./dist" | |||
"module": "ES2022", |
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.
Just confirming but this is to enable top-level await?
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.
Correct :)
docs-snippets
directory #3205Summary
docs-snippets
todocs
Checklist