feat(node/plugins): esbuild options

[high1]

Description

Since https://github.com/evanw/esbuild/releases/tag/v0.14.51 esbuild supports JSX transformations, so it makes sense that jsx and jsxImportSource are forwarded along with other relevant options

Additional context

This PR just extends the list of relevant fields that are forwarded to esbuild. Now, using voby without any plugins, it's needed to duplicate these options in vite configuration also, like here https://github.com/high1/voby-clock/blob/main/vite.config.ts. Using feature for this change is maybe incorrect, but it's also not a bug fix, or is it?

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[bluwy]

Note this was previously fixed in #10374 but was reverted in #10714. Might be a good time to reimplement the changes for Vite 4.

[high1]

Should this be closed, @bluwy ? Or should precedence be added to esbuild config, if it exists? Maybe something like

for (const field of meaningfulFields) {
  if (field in loadedCompilerOptions) {
    // @ts-ignore TypeScript can't tell they are of the same type
    compilerOptionsForFile[field] = (options?.[field]) ?? loadedCompilerOptions[field]
  }
}

options?.[field] would make sure that the option from vite config takes precedence.

[bluwy]

Yeah having the esbuild options take precedence over the ones in tsconfig makes sense to me. We could update this PR to re-apply #10374, or open a new PR (whichever you prefer). But maybe @sapphi-red has some thoughts about this before we move forward.

[sapphi-red]

Yeah, I think the same as @bluwy (esbuild options should take precedence over tsconfig ones).
Maybe other fields (jsxFactory, jsxFragmentFactory) needs to be aligned, too.

I think we should skip adding some values to compilerOptionsForFile[field] when options[field] existed, instead of what you showed. Because it isn't a one-to-one onto mapping (e.g. esbuild.jsx+esbuild.jsxDev corresponds to tsconfig.compilerOptions.jsx).

[patak-dev]

@high1 let us know if you plan to work on this feature. Our plan is to release the first beta of Vite 4 in a few days and cut the major around Dec 10th, and I agree with @bluwy and @sapphi-red that the major is a good opportunity to go ahead with this change. We could take this over if you are busy, there is no problem.

[high1]

@patak-dev I am planning to work, yes, and would be very excited to contribute. I was a bit overwhelmed with other stuff, but now I'm able to finish the PR. Considering what @sapphi-red said, would an acceptable course of action be to remove the features that are overriden in Vite configuration could from resolved TypeScript configuration? This would avoid the need of hand-mapping the properties, but I'm not sure if removing something here would have other unwanted effects?
Something like"

  options?.target && delete resolvedOptions.tsconfigRaw?.compilerOptions?.target
   options?.jsx && delete resolvedOptions.tsconfigRaw?.compilerOptions?.jsx
   options?.jsxFactory && delete resolvedOptions.tsconfigRaw?.compilerOptions?.jsxFactory
   options?.jsxFragment && delete resolvedOptions.tsconfigRaw?.compilerOptions?.jsxFragmentFactory
   options?.jsxImportSource && delete resolvedOptions.tsconfigRaw?.compilerOptions?.jsxImportSource

options are TransformOptions in src/node/plugins/esbuild. If I'm completely missing what should be done here, I would appreciate some guidance, @bluwy , @sapphi-red .

[sapphi-red]

@high1 That looks good to me!
Though, I recommend using foo.bar = undefined instead of delete foo.bar as delete is slow.

a quick benchmark

console.time('delete')
for (let i = 0; i < 1000; i++) {
  const foo = { bar: 'bar' }
  delete foo.bar
}
console.timeEnd('delete')

console.time('assign undefined')
for (let i = 0; i < 1000; i++) {
  const foo = { bar: 'bar' }
  foo.bar = undefined
}
console.timeEnd('assign undefined')

result on my machine
delete: 0.10302734375 ms
assign undefined: 0.02783203125 ms

[high1]

Updated the PR - added changes from #10374, increased esbuild version to ^0.15.11 to make sure that relevant options are defined and changed the implementation to set relevant TSConfigRaw options to undefined when defined in Vite configuration - this way, if both are defined, Vite configuration takes precedence.

[sapphi-red]

Would you add a test here to make sure removing tsconfig.compilerOptions works as we expect in future esbuild versions too?

vite/packages/vite/src/node/__tests__/plugins/esbuild.spec.ts

Lines 244 to 252 in c26679a

	describe('transformWithEsbuild', () => {
	test('not throw on inline sourcemap', async () => {
	const result = await transformWithEsbuild(`const foo = 'bar'`, '', {
	sourcemap: 'inline'
	})
	expect(result?.code).toBeTruthy()
	expect(result?.map).toBeTruthy()
	})
	})

[sapphi-red]

@high1 We merged this PR without tests to test it out. If you are not going to add a test, please let us know. I'll take it. If you are going to, no rush. 👍

[high1]

@sapphi-red I just didn't have the time - hoping to finish the tests this week.