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

fix(config): treat all files as ESM on deno #18081

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

marvinhagemeister
Copy link
Contributor

Description

The CJS detection logic that checks whether the CJS entry of vite itself is used is based around package.json detection. With Deno this assumption is not true anymore as most Deno projects don't have a package.json file. Instead they either have a deno.json or a deno.jsonc file. Projects in Deno are always in ESM too.

When using vite in Deno you'd always see this warning unless you added a package.json with type: module:

Screenshot 2024-09-11 at 14 48 08

Ultimately, this is caused by the check here

return pkg?.data.type === 'module'

Which infers that the current project is in CJS. This PR addresses that by making vite aware of Deno projects. When running inside Deno, we'll not just check for package.json, but also deno.json and deno.jsonc too.

Screenshot 2024-09-11 at 14 50 53

I'm not too familiar with the testing setup here in vite and would need some pointers for adding a test. Where do these go here?

Fixes denoland/deno#25574

Copy link

stackblitz bot commented Sep 11, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@sapphi-red
Copy link
Member

sapphi-red commented Sep 12, 2024

findNearestPackageData is a hot spot and probably should avoid adding fs accesses if possible.
Does having deno.json/deno.jsonc in node_modules change the behavior?
If it doesn't, I wonder if we can change this line

const isESM = isFilePathESM(resolvedPath)

to

const isESM = process.versions.deno || isFilePathESM(resolvedPath) 

instead.

@marvinhagemeister
Copy link
Contributor Author

marvinhagemeister commented Sep 12, 2024

Thanks for the tip! I had a feeling that putting the check in findNearestPackageData wasn't the right place. Your suggestion of putting it in the config loading logic is much better. Updated the PR.

EDIT: The test that fails in CI fails in main for me too.

@patak-dev patak-dev added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Sep 12, 2024
@sapphi-red sapphi-red changed the title fix(config): deno resolving vite cjs entry fix(config): treat all files as ESM on deno Sep 17, 2024
@sapphi-red sapphi-red merged commit c1ed8a5 into vitejs:main Sep 17, 2024
11 of 13 checks passed
@marvinhagemeister marvinhagemeister deleted the fix-deno-cjs branch September 17, 2024 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move from package.json to deno.json -> All Vite projects complain about deprecated CJS mode
3 participants