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

Set esbuild target as node12 #10061

Closed
wants to merge 1 commit into from
Closed

Conversation

mtsmfm
Copy link
Contributor

@mtsmfm mtsmfm commented Aug 28, 2021

Currently, target version is not specified so it's esnext.

https://esbuild.github.io/api/#target

As a result, generated file is not valid for node12, the node version which is used by theia editor.

https://github.com/eclipse-theia/theia/blob/02261581de60f5221de6264fab4b011c51e5ab91/package.json#L7

$ wget https://github.com/rust-analyzer/rust-analyzer/releases/download/2021-08-23/rust-analyzer.vsix
$ unzip rust-analyzer.vsix -d ext
$ node --version                                                                                           
v12.22.5
$ node --check ./ext/extension/out/main.js
...
void 0;var qd=_n(),Di=Fr(),aw=kd(),Od=Nr(),cw="Content-Length: ",Id=`\r
                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                            

SyntaxError: Unexpected token '?'
    at wrapSafe (internal/modules/cjs/loader.js:915:16)
    at checkSyntax (internal/main/check_syntax.js:66:3)
    at internal/main/check_syntax.js:39:3
$ node --version
v14.17.5
$ node --check ext/extension/out/main.js

@lnicola
Copy link
Member

lnicola commented Aug 28, 2021

We just removed a workaround for Node 12 in #10053, as the official policy is to support the latest two VS Code releases.

Is Theia planning to rebase on top of a newer Code version (looks like it's on 1.45 currently)? Do we really want to commit to supporting Node 12 indefinitely?

@mtsmfm
Copy link
Contributor Author

mtsmfm commented Aug 28, 2021

@lnicola Thank you for pointing out.
It seems theia is currently updating its node version.

eclipse-theia/theia#9936

Do we really want to commit to supporting Node 12 indefinitely?

Probably the PR will be released on the end of Sep. It may be postponed though, I think we don't need to support indefinitely.

https://github.com/eclipse-theia/theia/milestone/24

@lnicola
Copy link
Member

lnicola commented Aug 28, 2021

Any opinion, @matklad? Doing this would mean reverting #10053.

It probably doesn't matter too much for now (Node 12 is EOL in April 2022), but I feel this is going to happen again in the future.

@matklad
Copy link
Member

matklad commented Aug 28, 2021

Yeah, I'd rather put the pressure on upstream to update, rather than maintain compatibility downstream. We do need to say --target=node14 though explicitly!

@matklad
Copy link
Member

matklad commented Aug 28, 2021

As a remainder, in this kinds of situation there's always an option of using an older version -- it's not that rust-analyzer won't support theia at all, it's just that theia users would have to use an older version of rust-analyzer.

bors bot added a commit that referenced this pull request Aug 28, 2021
10062: Set esbuild target as node14 r=matklad a=mtsmfm

ref: #10061

Currently, target version is not specified so it's esnext.

https://esbuild.github.io/api/#target

VSCode uses node 14 since version 1.56.

#3167 (comment)

Co-authored-by: Fumiaki MATSUSHIMA <mtsmfm@gmail.com>
@mtsmfm
Copy link
Contributor Author

mtsmfm commented Aug 28, 2021

Yeah, I'd rather put the pressure on upstream to update, rather than maintain compatibility downstream. We do need to say --target=node14 though explicitly!

OK, I sent another PR to set target explicitly.
#10062

As a personal (and for theia users) memo, probably 2021-08-09 is the last version which supports nodejs 12 because previously tsc is used and its target is es2019

4638604

https://github.com/rust-analyzer/rust-analyzer/releases/tag/2021-08-09

@mtsmfm mtsmfm closed this Aug 28, 2021
@mtsmfm mtsmfm deleted the specify-target branch August 28, 2021 14:55
@lnicola
Copy link
Member

lnicola commented Aug 28, 2021

@mtsmfm I think #7284 would be a large improvement for Theia users, perhaps you'd like to look into it.

@mtsmfm
Copy link
Contributor Author

mtsmfm commented Aug 29, 2021

@lnicola I'm not familiar with Open VSX though, I don't think it's related to Theia.
Actually I use che-theia and it uses its own plugin registry.
https://github.com/eclipse-che/che-plugin-registry

@paul-marechal
Copy link

It seems theia is currently updating its node version.

Note that we are only removing unnecessary constraints on the upper bound Node version: we concluded that we should limit ourselves to using Node.js 12 JS API only (at least for now), this way adopters should be able to use any Node.js version >=12 thanks to backward compatibility of the Node.js APIs.

But what this issue shows me is that we can't really expect VS Code extension developers to limit themselves to older Node.js APIs, it only makes sense to align with VS Code's runtime (node 14 here).

Regarding your current configuration, as VS Code runs on Node.js 14 I don't think esnext is a great target: According to node.green esnext features aren't fully supported by node 14 so you might want to change it to something like es2020?

@lnicola
Copy link
Member

lnicola commented Aug 31, 2021

I assume Theia will have to move on in April, when Node 12 goes EOL.

As for esnext, we've already switched to node14 in #10062, which seems both reasonable and explicit enough.

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.

4 participants