Skip to content
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

feat: add new plugin #1319

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

fwx5618177
Copy link

@fwx5618177 fwx5618177 commented Apr 25, 2024

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

  • Add a new plugin, it named remark-codesandbox-sandpack.

@github-actions github-actions bot added the 👋 phase/new Post is being triaged automatically label Apr 25, 2024

This comment has been minimized.

@github-actions github-actions bot added 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Apr 25, 2024
- Change the description

Signed-off-by: wenxuan feng <279357596@qq.com>
@ChristianMurphy
Copy link
Member

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 prepack script 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 ⚠️, prepack script is missing ⚠️, and it isn't published to npm ⚠️ (https://www.npmjs.com/package/remark-codesandbox-sandpack)

More specific recommendations:

  1. 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, swc should not be included (instead devDependencies)
    • dependencies which might be used along side like react, react-markdown, remark should not be included in dependencies (instead devDependencies or peerDependencies)
  2. 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
  3. Avoid self referential imports https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/356192fa74277749cebb9d1c3876e69c79df7ccf/src/index.ts#L3 will likely break build/bundle tools
  4. 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
  5. 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
  6. Consider leveraging node:test (https://nodejs.org/api/test.html) or vitest (https://vitest.dev/) for running the test suite, they are fast, modern, and have solid support for ESM.
  7. 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤞 phase/open Post is being triaged manually
Development

Successfully merging this pull request may close these issues.

None yet

2 participants