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

Convert everything to ESM #349

Closed
Rich-Harris opened this issue Jan 29, 2021 · 18 comments
Closed

Convert everything to ESM #349

Rich-Harris opened this issue Jan 29, 2021 · 18 comments

Comments

@Rich-Harris
Copy link
Member

Modules are now fully supported in Node, except in Node 10 which will fall out of LTS very soon (the current 'active' LTS version is 14).

It makes sense, I think, for SvelteKit to fully embrace ESM, by which I mean that the CLI should be distributed as ESM, and that svelte.config.js and snowpack.config.js should be ESM.

There is a practical benefit to this, aside from making everything new and shiny and futureproof: it means that you can trivially share modules between the app you're building and e.g. npm run scripts. (We've encountered this at the NYT — we have a decompress module that is primarily used in the client to calculate things like rolling averages, which don't need to come over the wire, but which are also needed script-side to generate some summary data files.)

@Rich-Harris Rich-Harris added this to the public beta milestone Jan 29, 2021
@dummdidumm
Copy link
Member

I need to investigate how that affects the language server and vs code extension. Currently they only support cjs style. Related issue sveltejs/language-tools#509

@Rich-Harris
Copy link
Member Author

@dummdidumm how confident are you that it's possible? should it be considered a blocker?

@dummdidumm
Copy link
Member

Not sure at this moment. There were some ideas on how to accomplish it but ultimately I gotta test that out. I hope to get to this in the next few days.

@dummdidumm
Copy link
Member

dummdidumm commented Jan 30, 2021

I have hit multiple walls and I am not confident that this can be resolved at this point.

The current Electron (which powers VS Code) is at Node 12.x, so no native ESM support. This means we need to transpile the language-server code with module: "CommonJS". This means that await import(...) statements are transpiled to code using require which will fail because require cannot deal with ESM (not in 12, not in 14). Using the esm package is not possible currently because of this open PR which is needed for new syntax features like optional chaining or nullish coalescing (maybe that PR could be used to keep a local dist of esm, but I don't know if that's viable).

What's left (that I know of) is ts-node. I hijacked it by hooking the transpiler into regular js files so that the typescript transpiler converts the file into commonjs. It did work though with this snippet:

import { register } from 'ts-node';

// inside a custom cosmiconfig loader function:
const loader = (filePath: string) => {
 // ...
        register({
            transpileOnly: true,
            compilerOptions: { module: 'CommonJS', esModuleInterop: true, allowJs: true },
            skipProject: true
        });
// ...

The problem is that once ts-node is registered, it will be used on all files that are required afterwards which may not be what we want. EDIT: I just noticed I can disable this afterwards, so this is no longer an issue. This means this approach might actually work

Not sure if that applies in this case since everything is transpiled by TS to commonJS | Also, ESM-support in ts-node is still experimantal and they have to jump through quite some hoops to get it working, for example forking some of node's internals. Quoting the readme in there: "One obvious problem with this approach: the code has been pulled from one version of node, whereas users of ts-node run multiple versions of node. Users running node 12 may see that ts-node behaves like node 14, for example."

One alternative to circumvent all this is to say "dear Svelte Kit users, if you want your svelte.config.js to be understood by the language server, make a .cjs copy with the relevant parts". This is obviously not that user friendly.

@Rich-Harris
Copy link
Member Author

Instead of making a .cjs copy of the config file, what if the config file was always .cjs, until this issue is resolved? Would that bypass this mess?

@dummdidumm
Copy link
Member

You mean instead of svelte.config.js, SvelteKit will have a svelte.config.cjs with the config in it? From my veeery limited knowledge of CommonJS-ESM-interop it could work (ESM->CommonJS is ok if I read this correctly)
If that would work with the rest of the setup that would be a good solution.

@Rich-Harris
Copy link
Member Author

The interop is fine — you can import foo from './foo.cjs' in an ESM project and it will Do The Right Thing. So that seems viable

@benmccann
Copy link
Member

I think this is done at this point with the exception of #400. Feel free to reopen if I'm wrong though

@benmccann
Copy link
Member

Electron 12 uses Node v14 I believe (https://www.electronjs.org/releases/stable). Help > About shows VS Code is on Electron 11.3. Hopefully at some point it will be updated which my unblock language tools being able to use ESM

@GrygrFlzr
Copy link
Member

It says it's currently on Node 12.18.3, is that not compatible with ESM?

@babichjacob
Copy link
Member

It says it's currently on Node 12.18.3, is that not compatible with ESM?

If I remember correctly, only with the --experimental-modules flag.

@GrygrFlzr
Copy link
Member

According to the Node 12.17 changelog:

As of Node.js 12.17.0, the --experimental-modules flag is no longer necessary to use ECMAScript modules (ESM).

In fact, this is why our create-app template requires Node 12.17 as a minimum.

There seems to be an open issue about this: electron/electron#21457, in particular this interesting comment:

AFAIK, Electron does support esm, but not as the first file loaded.
You still need to have a cjs file at first, that will load your esm application

It's the exact same situation as the AWS lambdas! We only need the entry point to be CJS, and it can await import ESM from there!

@dummdidumm
Copy link
Member

Interesting, will do a spike if I have the time testing this. If it works I need to come up with a good solution for the async loading of the config though, right now it's expected to be synchronous because that code path needs to be synchronous,too.

@hatemjaber
Copy link

I've burnt too many hours trying to resolve this issue.... I'm using typeorm and having issues running a migration, I keep getting the following error:

`import type { MigrationInterface, QueryRunner } from "typeorm";
^^^^^^

SyntaxError: Cannot use import statement outside a module`

outside of compiling the ts to js and using the js file, I can't get this to work. I just want an out of the box experience without having to tweak the generated files. Am I doing something wrong? Is there something else that I need to do in order to get this to work? I assumed using the ts version when generating the project would give me all the settings that I need to run ts code. I put all my ts code inside the src/lib to avoid issues.

I apologize if this is not the right place to ask, I tried on discord but was not able to get an answer. i have everything else working with typeorm except the migrations which uses their cli and causes this error.

@benmccann
Copy link
Member

TypeOrm is known to have difficulties working with SvelteKit. See #798. Though there is a suggestion there of a Vite plugin to try using. No one has reported yet whether it works or not

@hatemjaber
Copy link

hatemjaber commented Jun 17, 2021

@benmccann I know what the issue is but I don't know how to resolve it without breaking sveltekit, maybe you can guide me.

If I remove "type": "module" from package.json and i change "module": "es2020" to "module": "commonjs" in tsconfig.json, it works.

Is there a way around this where I don't have to make these changes?

@hatemjaber
Copy link

hatemjaber commented Jun 17, 2021

@benmccann I'm sorry to bother you, maybe this helps...

I modified the recommendation in the typeorm documentation for adding the custom script by forcing it to use commonjs like so:
"typeorm": "TS_NODE_COMPILER_OPTIONS='{\"module\":\"commonjs\"}' node --require ts-node/register ./node_modules/typeorm/cli.js"

The only issue now is getting around the "type": "module". I don't know enough about sveltekit/typescript and how they work to get around that one. If you can tell me how to do that without breaking sveltekit, that would make using tyeporm possible.

Thank you in advance!

@hatemjaber
Copy link

hatemjaber commented Jun 17, 2021

This is a hack, please advise if you have a better solution:

"dev": "npx json -I -f package.json -e \"this.type='module'\" && svelte-kit dev"
"typeorm": "npx json -I -f package.json -e \"this.type='commonjs'\" && TS_NODE_COMPILER_OPTIONS='{\"module\":\"commonjs\"}' node --require ts-node/register ./node_modules/typeorm/cli.js" }

Rewrite the type each time we run an npm script

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

No branches or pull requests

6 participants