feat: add new plugin
Initial checklist
- [x] I read the support docs
- [x] I read the contributing guide
- [x] I agree to follow the code of conduct
- [x] I searched issues and couldn’t find anything (or linked relevant results below)
- [x] If applicable, I’ve added docs and tests
Description of changes
- Add a new plugin, it named remark-codesandbox-sandpack.
Hi! It seems some of the things asked in the template are missing? Please edit your post to fill out everything.
- [x] Initial checklist
- [ ] Description of changes (todo)
You won’t get any more notifications from me, but I’ll keep on updating this comment, and remove it when done!
If you need it, here’s the original template
<!--
Please check the needed checkboxes ([ ] -> [x]). Leave the
comments as they are, they won’t show on GitHub.
We are excited about pull requests, but please try to limit the scope, provide
a general description of the changes, and remember, it’s up to you to convince
us to land it.
-->
### Initial checklist
* [ ] I read the support docs <!-- https://github.com/remarkjs/.github/blob/main/support.md -->
* [ ] I read the contributing guide <!-- https://github.com/remarkjs/.github/blob/main/contributing.md -->
* [ ] I agree to follow the code of conduct <!-- https://github.com/remarkjs/.github/blob/main/code-of-conduct.md -->
* [ ] I searched issues and couldn’t find anything (or linked relevant results below) <!-- https://github.com/search?q=user%3Aremarkjs&type=Issues -->
* [ ] If applicable, I’ve added docs and tests
### Description of changes
TODO
<!--do not edit: pr-->
Thanks, — bb
Hey @fwx5618177! 👋 Thanks for sharing.
General feedback on what gets included in the plugins list:
Some criteria to include packages in this list.
- Some documentation with at least some instructions on how to use the package.
- A CI job to run tests. (The tests are already there.)
- A
prepackscript and CI job to build the types. (The JSDoc types are already there.)- The package should be published to npm.
source: https://github.com/syntax-tree/hast/pull/24#issuecomment-2015384111
It looks like documentation is there ✅ , CI is missing ⚠️, prepack script is missing ⚠️, and it isn't published to npm ⚠️ (https://www.npmjs.com/package/remark-codesandbox-sandpack)
More specific recommendations:
- limit your dependencies to those downstream users directly need to run your package https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/356192fa74277749cebb9d1c3876e69c79df7ccf/package.json#L51-L68
- dependencies that are for development only
typescript,tsx,swcshould not be included (instead devDependencies) - dependencies which might be used along side like
react,react-markdown,remarkshould not be included in dependencies (instead devDependencies or peerDependencies)
- dependencies that are for development only
- When importing only types from a package use
import type {}https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/356192fa74277749cebb9d1c3876e69c79df7ccf/src/index.ts#L2 to avoid side effects and clarify intent - Avoid self referential imports https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/356192fa74277749cebb9d1c3876e69c79df7ccf/src/index.ts#L3 will likely break build/bundle tools
- The type configuration could be greatly simplified by setting
"checkJs": true,,"module": "node16",, and"strict": true,. current: https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/356192fa74277749cebb9d1c3876e69c79df7ccf/tsconfig.json#L3-L41 example of simplified: https://github.com/remarkjs/remark-directive/blob/876a45ad4c021c74bb0b65383e838c8f3181e1eb/tsconfig.json#L1-L15 - Packages should generally be shipped as raw JS or as close to it as possible, avoid pre-bundling/pre-polyfilling/pre-minifying code for users, let their build tools add the parts they need. https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/356192fa74277749cebb9d1c3876e69c79df7ccf/package.json#L23
- Consider leveraging
node:test(https://nodejs.org/api/test.html) orvitest(https://vitest.dev/) for running the test suite, they are fast, modern, and have solid support for ESM. - Do type check the test suite, this is how users will see the types, if the types are wrong and broken in the tests, they will be for users as well https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/356192fa74277749cebb9d1c3876e69c79df7ccf/tests/utils.spec.ts#L1
:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 100.00%. Comparing base (
a185821) to head (87f67cb). Report is 16 commits behind head on main.
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@ Coverage Diff @@
## main #1319 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 6 6
Lines 142 142
=========================================
Hits 142 142
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hey @fwx5618177! 👋 Thanks for sharing.
General feedback on what gets included in the plugins list:
Some criteria to include packages in this list.
- Some documentation with at least some instructions on how to use the package.
- A CI job to run tests. (The tests are already there.)
- A
prepackscript and CI job to build the types. (The JSDoc types are already there.)- The package should be published to npm.
source: syntax-tree/hast#24 (comment)
It looks like documentation is there ✅ , CI is missing ⚠️,
prepackscript is missing ⚠️, and it isn't published to npm ⚠️ (https://www.npmjs.com/package/remark-codesandbox-sandpack)More specific recommendations:
limit your dependencies to those downstream users directly need to run your package https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/356192fa74277749cebb9d1c3876e69c79df7ccf/package.json#L51-L68
- dependencies that are for development only
typescript,tsx,swcshould not be included (instead devDependencies)- dependencies which might be used along side like
react,react-markdown,remarkshould not be included in dependencies (instead devDependencies or peerDependencies)When importing only types from a package use
import type {}https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/356192fa74277749cebb9d1c3876e69c79df7ccf/src/index.ts#L2 to avoid side effects and clarify intentAvoid self referential imports https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/356192fa74277749cebb9d1c3876e69c79df7ccf/src/index.ts#L3 will likely break build/bundle tools
The type configuration could be greatly simplified by setting
"checkJs": true,,"module": "node16",, and"strict": true,. current: https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/356192fa74277749cebb9d1c3876e69c79df7ccf/tsconfig.json#L3-L41 example of simplified: https://github.com/remarkjs/remark-directive/blob/876a45ad4c021c74bb0b65383e838c8f3181e1eb/tsconfig.json#L1-L15Packages should generally be shipped as raw JS or as close to it as possible, avoid pre-bundling/pre-polyfilling/pre-minifying code for users, let their build tools add the parts they need. https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/356192fa74277749cebb9d1c3876e69c79df7ccf/package.json#L23
Consider leveraging
node:test(https://nodejs.org/api/test.html) orvitest(https://vitest.dev/) for running the test suite, they are fast, modern, and have solid support for ESM.Do type check the test suite, this is how users will see the types, if the types are wrong and broken in the tests, they will be for users as well https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/356192fa74277749cebb9d1c3876e69c79df7ccf/tests/utils.spec.ts#L1
All done!
At a high level, it looks like this plugin aims to work on HTML? If so it may make sense as a rehype plugin instead, where is can work more safely on the HTML AST.
Some specific comments.
-
It can be good to have an interface for properties, this both makes code easier to read, and allows you to export the type if wanted https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/7eda3d772a7b73a32ce8303d6bf23be091659391/src/htmlTemplate.ts#L1-L16
-
Avoid
anytype when possible, a more specific type for what a template or theme will help ensure correct usage https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/7eda3d772a7b73a32ce8303d6bf23be091659391/src/htmlTemplate.ts#L3-L4 -
Would it make sense to allow the react or sandpack sources and versions to be configurable? https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/7eda3d772a7b73a32ce8303d6bf23be091659391/src/htmlTemplate.ts#L26-L29
-
When there are type only imports it can be good to use
import type { ... }to prevent side effects https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/7eda3d772a7b73a32ce8303d6bf23be091659391/src/runtimeEnv.ts#L2 -
Is it intentional that documentation switches between languages? https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/7eda3d772a7b73a32ce8303d6bf23be091659391/src/utils.ts#L50-L52
-
For module resolution that works in the most places you probably want to use
Node16https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/7eda3d772a7b73a32ce8303d6bf23be091659391/tsconfig.json#L5-L7A common misconception is that node16 and nodenext only emit ES modules. In reality, node16 and nodenext describe versions of Node.js that support ES modules, not just projects that use ES modules. Both ESM and CommonJS emit are supported, based on the detected module format of each file. Because node16 and nodenext are the only module options that reflect the complexities of Node.js’s dual module system, they are the only correct module options for all apps and libraries that are intended to run in Node.js v12 or later, whether they use ES modules or not.
https://www.typescriptlang.org/docs/handbook/modules/reference.html#the-moduleresolution-compiler-option
-
Transitive dependencies probably should have valid typings, do you see an error if you enable lib check? https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/7eda3d772a7b73a32ce8303d6bf23be091659391/tsconfig.json#L19-L20
-
It is best to check the types in the tests, this ensures users can use the library with TypeScript https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/7eda3d772a7b73a32ce8303d6bf23be091659391/tests/utils.spec.ts#L1
-
In the GitHub action, consider using the official pnpm installer https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/7eda3d772a7b73a32ce8303d6bf23be091659391/.github/workflows/release.yml#L29, official installer https://github.com/pnpm/action-setup
-
It seems a bit surprising to manually generate a tarball, wouldn't
npmorpnpmgenerate this? https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/7eda3d772a7b73a32ce8303d6bf23be091659391/.github/workflows/release.yml#L45-L47 -
Commitlint can be very useful for checking commits and pr titles https://commitlint.js.org/ https://commitlint.js.org/guides/ci-setup.html#github-actions A snippet which may be of interest
name: Check PR Title on: pull_request: types: - opened - reopened - edited jobs: pr: runs-on: ubuntu-latest timeout-minutes: 1 steps: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: node-version: 20 - run: echo "$TITLE" | npx commitlint env: TITLE: ${{ github.event.pull_request.title }}
There’s some standards attached with this list of plugins we recommend. Looks like there’s still a bunch of open suggestions. Feel free to raise another PR when this is ready. Or open discussions / chat here when you have questions!
Hi! This was closed. Team: If this was merged, please describe when this is likely to be released. Otherwise, please add one of the no/* labels.