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 3 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

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (a185821) to head (87f67cb).
Report is 5 commits behind head on main.

❗ 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           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fwx5618177
Copy link
Author

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

All done!

@ChristianMurphy
Copy link
Member

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.

  1. 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

  2. Avoid any type 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

  3. 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

  4. 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

  5. Is it intentional that documentation switches between languages? https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/7eda3d772a7b73a32ce8303d6bf23be091659391/src/utils.ts#L50-L52

  6. For module resolution that works in the most places you probably want to use Node16 https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/7eda3d772a7b73a32ce8303d6bf23be091659391/tsconfig.json#L5-L7

    A 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

  7. 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

  8. 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

  9. 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

  10. It seems a bit surprising to manually generate a tarball, wouldn't npm or pnpm generate this? https://github.com/fwx5618177/remark-codesandbox-sandpack/blob/7eda3d772a7b73a32ce8303d6bf23be091659391/.github/workflows/release.yml#L45-L47

  11. 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 }}

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

3 participants