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

Add support for stopping the ESBuild service on Webpack shutdown #354

Closed
wants to merge 2 commits into from

Conversation

Cyberboss
Copy link

Calling the new stop API mentioned in the release notes: https://github.com/evanw/esbuild/releases/tag/v0.19.11

Cyberboss added a commit to Cyberboss/tgstation that referenced this pull request Jan 16, 2024
Workaround for ESBuild lingering until privatenumber/esbuild-loader#354 is merged
@@ -48,7 +48,7 @@
"webpack": "^4.40.0 || ^5.0.0"
},
"dependencies": {
"esbuild": "^0.19.0",
"esbuild": "^0.19.11",
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
"esbuild": "^0.19.11",
"esbuild": "^0.19.0",

Revert this back.
The lock file can use the latest esbuild, but this specifies the supported range of esbuild that users can install.

@privatenumber
Copy link
Owner

privatenumber commented Jan 17, 2024

Can you tell me more about the motivation of this change?

Few thoughts:

  • In most cases, this shouldn't be necessary as the loader is loaded by Webpack to build and exit. I'm thinking it only makes sense if Webpack is loaded via Node API.
  • Does this exit in Webpacks watch mode?
  • Will esbuild be re-spawned if used after exit is called?
  • We allow custom esbuild to be passed in. So this stop method will have to be supported there too. Maybe update implementation to accept the entire esbuild export (e.g. import * as esbuild from 'esbuild') Note this will need to be backwards compatible with just passing in transform.

@privatenumber
Copy link
Owner

I'm pulling out the esbuild upgrade here as the tests need to be updated: #355

@Cyberboss Cyberboss marked this pull request as draft January 20, 2024 23:39
@privatenumber
Copy link
Owner

Closing due to lack of activity

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.

2 participants