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

Support ESM Syntax in config files #509

Closed
dummdidumm opened this issue Aug 31, 2020 · 7 comments
Closed

Support ESM Syntax in config files #509

dummdidumm opened this issue Aug 31, 2020 · 7 comments
Labels
feature request New feature or request Fixed Fixed in master branch. Pending production release.

Comments

@dummdidumm
Copy link
Member

Is your feature request related to a problem? Please describe.
Config files need to be written in node-commonjs-style (no import ... or export const ... syntax) because we use require inside our extension to dynamically load dependencies. This is confusing to users and not obvious.

Describe the solution you'd like
Change the setup so that ESM syntax is supported.

  • Option 1: through some build/bundle step of the extension where a bundler might help by doing its magic (related: Please can you optimize the size of the VSCode extension #139 )
  • Option 2: switch to the await import(...); syntax, which is problematic because svelte-preprocess is looked up inside Document creation. That can't be made asynchronous because it's used inside the TS LS (same problem as for Bug: DocumentSnapshot.ts is not passing preprocessed markup to svelte2tsx  #339 ).
    • a) rework how we interact with the TS LS. Codesandbox Client might help for inspiration here (afaik they have to deal with asynchronousity, too)
    • b) move svelte-preprocess-call out of Document and into the start of the application. This would be a breaking change as you now can only have one top-level svelte.config.js and not multiple at different levels - but I doubt this will break anyone, and it would also align more with the semantics of other config files (only one per project).

Help and other ideas highly appreciated

@orta
Copy link
Contributor

orta commented Sep 3, 2020

The is TS around in the dep tree, could transpile JS to common JS with it? (ts.transpileModule should be fast enough that this isn't a perf hit )

@dummdidumm
Copy link
Member Author

I gave it a try and I am not confident that this can be resolved at this point without something that feels like a hack.

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
        });
// ...

Once ts-node is registered, it will be used on all files that are required afterwards which may not be what we want. Fortunately we can enable/disable this at will. This means this approach might work, but it feels kind of hacky.

@nathanblair
Copy link

nathanblair commented Mar 6, 2021

https://github.com/sveltejs/language-tools/tree/master/packages/svelte-vscode#sveltelanguage-serverruntime

Could something like this be used to get around the limitation of VS Code using an Electron powered by Node 12 (seriously vs code team...node 14 has been LTS for almost a year now :D)?

This way is super hacky though, so I agree, I'm not sure it can be resolved at this point with a way that doesn't feel like a hack.

@alexanderniebuhr
Copy link

alexanderniebuhr commented Apr 3, 2021

why not just using import()?
this should work with common JS and ES Modules too. am I missing something?

for reference: https://github.com/windicss/windicss-intellisense/blob/b44056cad99cbe7819e6e765717ea154075d186f/src/lib/core.ts#L25

@dummdidumm
Copy link
Member Author

Like mentioned above, import() is wrongfully transpiled by TS, but someone found a good workaround which I'm using. So there's a pending PR now #930

@alexanderniebuhr
Copy link

Ok cool. Strange it seems it does not has any issues with ts for us

@dummdidumm
Copy link
Member Author

dummdidumm commented Apr 3, 2021

Because your compile target isn't commonjs from what I can see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request Fixed Fixed in master branch. Pending production release.
Projects
None yet
Development

No branches or pull requests

4 participants