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

minor: Update npm deps #9179

Merged
merged 21 commits into from
Jul 12, 2021
Merged

minor: Update npm deps #9179

merged 21 commits into from
Jul 12, 2021

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented Jun 8, 2021

No description provided.

@lnicola
Copy link
Member Author

lnicola commented Jun 8, 2021

r? @matklad

I didn't update @rollup/plugin-commonjs all the way because of a strange error:

The language client requires VS Code version ^1.53.0 but received version 1.56.2.

We might as well switch to esbuild, which seems to be the most liked bundler these days /s.

@lnicola lnicola marked this pull request as draft June 8, 2021 17:00
Comment on lines 3 to 6
!out
out/**
!out/src
!out/src/main.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what exactly are we pulling in from out?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The out/src dir. By the way !out/src/main.js is not necessary if you have !out/src but not out/src/**.

@lnicola
Copy link
Member Author

lnicola commented Jun 14, 2021

Status: this is marked as a draft because it breaks loading the extension. I didn't notice it because there's no error thrown.

@lnicola lnicola marked this pull request as ready for review July 10, 2021 15:37
@lnicola
Copy link
Member Author

lnicola commented Jul 10, 2021

r? @matklad

@matklad
Copy link
Member

matklad commented Jul 10, 2021

Scheduled for r+ on Monday!

@matklad
Copy link
Member

matklad commented Jul 12, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 12, 2021

@bors bors bot merged commit 745be39 into rust-lang:master Jul 12, 2021
@lnicola lnicola deleted the npm-deps branch July 12, 2021 14:54
@lnicola lnicola changed the title Update npm deps minor: Update npm deps Jul 13, 2021
@yaymukund
Copy link
Contributor

yaymukund commented Aug 8, 2021

I didn't update @rollup/plugin-commonjs all the way because of a strange error:

The language client requires VS Code version ^1.53.0 but received version 1.56.2.

We might as well switch to esbuild, which seems to be the most liked bundler these days /s.

Very mysterious bug.

It's worth noting that this blocks updating rollup itself. If you update to rollup@2.55 without @rollup/plugin-commonjs@19, you run into rollup/rollup#4195 . I encountered this while trying to package this extension for Nix.

Sorry to ping you on this old issue, I just thought it might save you some debugging in the future because it was fairly tricky to chase down.

@lnicola
Copy link
Member Author

lnicola commented Aug 8, 2021

Thanks for the ping, though I'll probably forget by then.


Anyway, as a non-web developer, the whole JS bundler ecosystem seems Kafkaesque.

@matklad
Copy link
Member

matklad commented Aug 8, 2021

One morning, as Gregor Samsa was waking up from anxious dreams, he discovered that his bed had been moved into node_modules.

@matklad
Copy link
Member

matklad commented Aug 8, 2021

Curiously, the docs now put esbuild to the first place:

https://code.visualstudio.com/api/working-with-extensions/bundling-extension#using-esbuild

I wonder if we should switch to that? On the one hand, I wouldn't want to pull yet another ecosystem (esbuild is in go), on the other hand, the pain with rollup/webpack is real, and the example in the docs does seem nice.

Fun fact: esbuild could have been written in Rust, but there was this blockin issue.

@yaymukund
Copy link
Contributor

yaymukund commented Aug 8, 2021

One morning, as Gregor Samsa was waking up from anxious dreams, he discovered that his bed had been moved into node_modules.

horror! 😂

I can try to integrate esbuild in the coming week, if this is something you feel is worth pursuing.

Also, that is a neat bit of trivia about esbuild. I wonder if swc has ever run into the same thing.

@lnicola
Copy link
Member Author

lnicola commented Aug 8, 2021

I can try to integrate esbuild in the coming week, if this is something you feel is worth pursuing.

If it means less npm deps in package.json, yes!

npm i --save-dev esbuild

Oh.

@yaymukund
Copy link
Contributor

If it means less npm deps in package.json, yes!

It would still reduce the number of dependencies because it has no dependencies of its own.

I wonder if we should switch to that? On the one hand, I wouldn't want to pull yet another ecosystem (esbuild is in go), on the other hand, the pain with rollup/webpack is real, and the example in the docs does seem nice.

For what it's worth, it wouldn't require Go as a build dependency. npm just pulls the static binary from the registry.

bors bot added a commit that referenced this pull request Aug 10, 2021
9832: vscode extension: use esbuild instead of rollup. r=matklad a=yaymukund

This shaves a couple seconds off our build & trims npm dependencies. I tested it using VSCode's "Install [Extension] from VSIX" option and it seems to work. Note that it changes the root of the package from `out/src/main → out/main`.

Fell out of the discussion in #9179 

Co-authored-by: Mukund Lakshman <yaymukund@gmail.com>
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.

None yet

4 participants