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(templates): enable ESM in all templates #4293

Closed
wants to merge 1 commit into from

Conversation

Mesteery
Copy link
Contributor

Description

ESM is not enabled in most templates, which causes an error when the project is started (except for Svelte, fixed by #3835).

import { defineConfig } from 'vite'
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at Object.compileFunction (node:vm:352:18)
    at wrapSafe (node:internal/modules/cjs/loader:1025:15)
    at Module._compile (node:internal/modules/cjs/loader:1059:27)
    at Module._extensions..js (node:internal/modules/cjs/loader:1124:10)
    at Object.require.extensions.<computed> [as .js] (...\node_modules\vite\dist\node\chunks\dep-11db14da.js:75452:13)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:816:12)
    at Module.require (node:internal/modules/cjs/loader:999:19)
    at require (node:internal/modules/cjs/helpers:93:18)
    at loadConfigFromBundledFile (...\node_modules\vite\dist\node\chunks\dep-11db14da.js:75457:17)14da.js:75457:17)

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@Shinigami92 Shinigami92 added the p2-nice-to-have Not breaking anything but nice to have (priority) label Jul 17, 2021
@Shinigami92
Copy link
Member

I wouldn't count that as a bug-fix but as a feature request due to you are always able to set this on your own after initial setup

@dsonet
Copy link
Contributor

dsonet commented Jul 17, 2021

I wouldn't count that as a bug-fix but as a feature request due to you are always able to set this on your own after initial setup

It should be able to work well after setup without throwing exception and need further configures.

@Shinigami92
Copy link
Member

It should be able to work well after setup without throwing exception and need further configures.

Could you provide a reproducible?
In example: I never had issues with template-vue-ts 🤔
Even I think I read something about that lit-element doesn't work with it currently
So I currently question myself if you had even tested all this?!

@Mesteery
Copy link
Contributor Author

Even I think I read something about that lit-element doesn't work with it currently

Yes, it works. I had this problem with lit-element, and by activating ESM, it worked

So I currently question myself if you had even tested all this?!

Only lit-element, but if you want, I can test them all

@Shinigami92
Copy link
Member

Only lit-element, but if you want, I can test them all

That would be nice 🙂

@Mesteery
Copy link
Contributor Author

Mesteery commented Jul 17, 2021

  • Vanilla / Vanilla TS

  • React / React TS

  • Lit Element / Lit Element : Uncaught SyntaxError: import not found: default error in the browser, same as below, but the page isn't blank

  • Vue / Vue TS : blank page & Uncaught SyntaxError: import not found: default in the browser (also reproducible with CJS, the problem does not come from this PR)

  • Preact / Preact TS (Migrate to ESM preactjs/preset-vite#16). There is also the Uncaught SyntaxError: import not found: default error in the browser, same as above (& blank page)

@haoqunjiang
Copy link
Member

How did you test these templates? Any code examples?

@Mesteery
Copy link
Contributor Author

I used my create-vite (with this commit).
node ../path/to/vite/packages/create-vite/index.js for each template.

@haoqunjiang
Copy link
Member

How did you test these templates? How are the errors thrown?

@Mesteery
Copy link
Contributor Author

I installed the dependencies and ran npm run dev for each template.
For Preact, the error is thrown when I npm run dev. For the rest it is in the browser console.

@haoqunjiang
Copy link
Member

That doesn't sound right. I never heard that the vite templates is failing right after scaffolding. Could you please open an issue for this problem? I think I need more context for this situation.

@Shinigami92
Copy link
Member

Please also see e.g. this issue #4307
So if we change it in every template suddenly it will break unexpected things
I'm not very quite satisfied at the moment with this PR/change 😬

@Mesteery
Copy link
Contributor Author

Yes I know, the ESM support of Yarn PnP is still in draft.

It is always possible for them to disable ESM anyway. And it's only a minority that will have to switch to CJS.

@haoqunjiang
Copy link
Member

I've seen issues caused by type: "module", but I still don't know what exactly this PR fixes.
Could you please also open an issue for the problem in your original PR description?

@haoqunjiang
Copy link
Member

My hesitations on this PR:

  1. I don't know what it fixes, because there are no related bug reports
  2. I don't know what and how many bugs it might bring, because there are already other issues people are complaining about, yet there's Import error in the browser and blank page in some templates (subst) #4302 that I can't even reproduce. It's a risk.

@Mesteery
Copy link
Contributor Author

I don't know what it fixes, because there are no related bug reports

I couldn't use Lit Element without enabling ESM, which should have been done in the template.

@haoqunjiang
Copy link
Member

I couldn't use Lit Element without enabling ESM

Which I never heard of. Please open an issue before submitting the fix.

@Mesteery
Copy link
Contributor Author

Indeed, this error (SyntaxError: Cannot use import statement outside a module) did not come from the templates, but from a symlink.
#4302 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants