Skip to content

feat!: move to ESM-only (CJS generation format also dropped) #448

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

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

Conversation

userquin
Copy link
Member

@userquin userquin commented Mar 29, 2025

Description

This PR includes the following changes:

  • move the repo to ESM-only: type now is module
  • removed format from SSG options, CJS output format removed
  • use resolve instead join on the build module to avoid problems with Windows file system
  • resolve server entry point in build module using pathToFileURL(resolve(...)).href removing the prefix and win32 check
  • removed client argument from createApp function (client, spa and build modules)
  • renamed eslint.config.mjs to eslint.config.js
  • changed all package exports to use .mjs without types (typesVersions still there to support Node10)
  • updated unbuild config declaration to node16 and removed rollup.emitCJS
  • docs: update readme file including ESM-only from v27.0.0 (CJS generation format also dropped). at top and new tree-shaking section
  • deprecate isClient

Will require changes from this PR #447 to test this PR in local examples...

Linked Issues

Additional context

From this comment #442 (comment)

@userquin userquin marked this pull request as ready for review March 29, 2025 15:36
@userquin userquin changed the title feat!: move to ESM-only feat!: move to ESM-only (CJS generation format also dropped) Mar 29, 2025
@antfu antfu requested a review from Copilot March 30, 2025 02:27
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR transitions the project to ESM-only by removing support for CJS generation, updating file system path handling, and revising function signatures to simplify usage. Key changes include:

  • Removal of the "format" option and related CJS logic in the configuration and build process.
  • Refactoring of path functions to use resolve and pathToFileURL for consistency and Windows compatibility.
  • Updates to client-side entry functions and documentation to reflect the ESM-only approach.

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/types.ts Removed the "format" option from ViteSSGOptions to enforce ESM-only mode.
src/node/build.ts Updated path handling and removed CJS-specific logic; adjusted createApp signature.
src/client/single-page.ts Simplified createApp by removing the unused client parameter.
src/client/index.ts Simplified createApp function signature by removing the client parameter.
build.config.ts Updated build configuration for ESM-only mode and removed emitCJS option.
README.md Updated docs to announce ESM-only support and added a tree-shaking guide.
Files not reviewed (1)
  • package.json: Language not supported

antfu and others added 2 commits March 30, 2025 10:32
@userquin
Copy link
Member Author

userquin commented Mar 30, 2025

don't merge yet, I'm going to update pwa plugin to 1.0.0, I'm having some issues with workbox-* deps resolving to lower 7.0.0 and should use 7.3.0 once pwa plugin updated to 1.0.0

done

@userquin userquin requested a review from antfu March 30, 2025 14:19
@userquin
Copy link
Member Author

userquin commented Mar 30, 2025

We'll need to add some test to test Rollup tree-shaking using isClient in some example.

@kevinmarrec iirc you want to add more tests here, we should check previous comment before releasing this 🤞

@userquin
Copy link
Member Author

userquin commented Mar 31, 2025

NOTE: @kevinmarrec the isClient will not be removed, what I want to test is the code behind !ctx.isClient being tree-shaked from client asset, and should work using (ctx) => { if (!ctx.isClient) console.log('SERVER') }

ofc, if isClient can also be removed it would be nice => in my previous example, the callback should be also removed since there is no code to run in the client, I guess Rollup cannot three-shake that (maybe adding #__NO_SIDE_EFFECTS__ annotation to the hook will work, we need to check the side effects 🤔 => rollup/rollup#5024 )

@kevinmarrec
Copy link
Contributor

kevinmarrec commented Mar 31, 2025

@userquin Please check this REPL and play with constants.js (simulated import.meta.env.SSR)

I think we should drop isClient and enforce import.meta.env.SSR in userland which is documented as the way to tree shake things anyway when dealing with client vs server code branches.

@userquin
Copy link
Member Author

userquin commented Mar 31, 2025

so, we just need to remove the entry from the readme here, thx

@kevinmarrec
Copy link
Contributor

kevinmarrec commented Mar 31, 2025

Tested this PR against my project, went smoothly, and won additional 0.01 Kb on client bundle size 🙈 thanks to

removed client argument from createApp function (client, spa and build modules)

@userquin
Copy link
Member Author

We should deprecate the isClient from the types.

@kevinmarrec
Copy link
Contributor

We should deprecate the isClient from the types.

Makes sense, isClient deprecation annotation + instruction "Use !import.meta.env.SSR instead"

@userquin
Copy link
Member Author

We should deprecate the isClient from the types.

Makes sense, isClient deprecation annotation + instruction "Use !import.meta.env.SSR instead"

We should add also a @see <href> with the link to the section in the readme

Copy link
Contributor

@kevinmarrec kevinmarrec left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants