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

Standalone content-tag implemented via conditional exports in package.json #44

Merged
merged 5 commits into from
Dec 12, 2023

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Dec 1, 2023

Alternative to: #41

Changes:

  • there are now two wasm-pack builds, one targeting noed, and one "browser" or "standalone" (as browser targets tend to be "standalone" / runnable anywhere) -- if we didn't need to support CJS, we could get away with only the standalone build
  • the root package.json is now the package.json that is published
    • now has name, description, etc
    • files entry includes the pkg directory
    • has exports with conditional/subpath entries to target node/browser separately since wasm-pack generates very different output for each target (there isn't a need for this if they went for full async-only API, but 🤷 )
  • To test that the browser changes actually work, vite has been added to devDependencies as well as an index.html
    • I tried to automate browser testing with vitest's browser mode: https://vitest.dev/guide/browser#browser-option-types -- but the browser mode does not support the same work around that vite has for Packages using self-referencing imports will fail to resolve ("moduleResolution": "node16" and package.json "exports") vitejs/vite#9731 -- which is setting resolve-alias for "package-name": "." (current directory) so that vite can resolve the "self's" exports. So we currently don't have automated browser testing atm. We could set up qunit instead of vitest, but we currently need to handroll a reporter to the CLI, because vite/vitest haven't given us those APIs to build custom reporters. GlimmerVM is already doing this, so it "just" needs to be extracted.
      This is the error:
      [plugin:vite:import-analysis] Failed to resolve import "content-tag" from "test-browser/process.test.js". Does the file exist?
      /home/nvp/Development/OpenSource/emberjs/content-tag/test-browser/process.test.js:2:48
      1  |  import { __vi_inject__ as __vi_esm_0__ } from 'vitest'
      2  |  import { __vi_inject__ as __vi_esm_1__ } from 'content-tag'
         |                                                 ^
      3  |  
      4  |
      
    • Due to not being able to correctly test with vitest, I added publint and @arethetypeswrong/cli to help me be more confident that the package.json was correctly configured so we don't have to rapid publishes to fix bugs with package.json config.
      @arethetypeswrong/cli reported some issues which are resolved now.
      One being this issue which is resolved by using node newer than 16 -- which... we don't declare node support at all, and node 16 is EOL'd so it seems fine to bump the CI's node.

In a real app

because my confidence in non-monorepo layouts is way lower (can't test with real projects with real module resolution),

here is a real usage:

@NullVoxPopuli NullVoxPopuli force-pushed the non-monorepo branch 3 times, most recently from ae7baeb to 50e660c Compare December 1, 2023 20:47
@@ -1,20 +1,45 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the root package.json now is the package.json that is published. previously, a separate package.json was used

"exports": {
".": {
"types": "index.d.ts",
"browser": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this conditional export is what enables browser mode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(assuming vite isn't falling back to the "default" exports (pkg/node/content_tag.cjs) and polyfilling all the node stuff for the browser)

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review December 1, 2023 22:13
cache: 'npm'
- run: npm ci
- run: ./build.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice to have this consistent with release workflow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the release workflow already run npm run build, so this is covered

@void-mAlex
Copy link
Contributor

void-mAlex commented Dec 1, 2023 via email

@ef4 ef4 merged commit 3b659f6 into main Dec 12, 2023
1 check passed
@NullVoxPopuli NullVoxPopuli added the enhancement New feature or request label Dec 13, 2023
@github-actions github-actions bot mentioned this pull request Dec 13, 2023
NullVoxPopuli added a commit to NullVoxPopuli/limber that referenced this pull request Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants