Include serialport bindings in package

[colin-grant-work]

This PR reduces the effects of #96 to the following:

• Replaces Webpack with ESBuild (but only use it on the webview code, as before Proposal: Package Extension #96)
• Move the output directory to dist rather than out. This is so that any adjustments made since Proposal: Package Extension #96 continue to apply. It would easy to move back to out if that is preferred.

In particular:

• We go back to relying on vsce's resolution of runtime depndencies to keep all of the node modules that are relevant at runtime.
• We use tsc to transpile into the dist folder, with .d.ts files in the expected 1:1 relationship with transpiled .js files.

Functionally, bundling worked fine for three of the four things to which it was applied, the extension code: the webview code, and the CDT-GDB Adapter's debugAdapter entry point. It failed for targetDebugAdapter because it depends on a package that depends on a package that requires specific file structure layout. Bundling the extension and the debugAdapter would bring some performance benefits, but they would be fairly negligible.

To Test

1. Build the plugin.
2. Run the plugin and execute a debug session with a remote target.
3. Run vsce ls and confirm that the kitchen sink is back (but that nothing from the dist folder we don't want has crept in).

Fixes #111

[eclipse-cdt-bot]

Can one of the admins verify this patch?

[colin-grant-work]

It looks like it may end up dragging other dependencies along with it, since removing it from the bundling tree cuts those out, as well. That would be unfortunate, as we'd be back to bundling hundreds of files for fear of missing some transitive dependency. I'll take a closer look.

[colin-grant-work]

Relevant here: prebuild/node-gyp-build#53.

[jonahgraham]

Thanks @colin-grant-work for your efforts so far! I tried out what you have and I see what you mean about it needing to pull in more and more.

FTR this is the error I get with the current version of the PR:

$ node dist/debugTargetAdapter.js --server=4711
node:internal/modules/cjs/loader:959
  throw err;
  ^

Error: Cannot find module 'debug'
Require stack:
- /tmp/qwe/extension/node_modules/@serialport/bindings-cpp/dist/index.js
- /tmp/qwe/extension/dist/debugTargetAdapter.js
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:956:15)
    at Function.Module._load (node:internal/modules/cjs/loader:804:27)
    at Module.require (node:internal/modules/cjs/loader:1028:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/tmp/qwe/extension/node_modules/@serialport/bindings-cpp/dist/index.js:22:33)
    at Module._compile (node:internal/modules/cjs/loader:1126:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1180:10)
    at Module.load (node:internal/modules/cjs/loader:1004:32)
    at Function.Module._load (node:internal/modules/cjs/loader:839:12)
    at Module.require (node:internal/modules/cjs/loader:1028:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/tmp/qwe/extension/node_modules/@serialport/bindings-cpp/dist/index.js',
    '/tmp/qwe/extension/dist/debugTargetAdapter.js'
  ]
}

IIUC correctly it is complaining about debug package being missing, because the unminified @serialport requires debug but it can't see the minified version that is already in the debugTargetAdapter.js.

[colin-grant-work]

@jonahgraham, I think my inclination at this point is to significantly scale back the ambition of #96 and just replace the bundling stage for the webview code with ESBuild, leaving the rest of the build more or less as it was. I think there's no way around including the whole dependency tree of @serialport/bindings-cpp, and if that's the case, then it seems that the only practical solution is just not trying to bundle anything - trying to isolate that sub-tree would be more trouble than its worth. And it also seems that Renesas quasi-NPM use case is really best served with a tsc-like compilation for the plugin-side code, so we might as well return to that if we're not going to get the benefits of bundling anyway. It would still get rid of Webpack, which was the most time-consuming part of the build, so it's not a total loss, but I likely should have started small.

Unless you think I'm being too pessimistic or you see a way around the dynamic import problems that the node-gyp-build bindings introduce, I'll update this PR to remove most of the bundling in the morning.

[colin-grant-work]

@asimgunes, in view of the problems raised in #111, this PR reverts many of the changes in #96 in a way that should be friendly to your use case. Relative to #107, it has most of what you've brought in, apart from separate esm and cjs modules. Is providing both important to your use case?

[asimgunes]

Hi, this request and PR #107 contains conflicted changes in the esbuild script. we might consider a change priority and rebase the changes accordingly.

[asimgunes]

Just a subjective comment: I'd consider using the old format where first excluding everything and including what is necessary here, it sound you have more control what is in the output. Only exclude what is not necessary results easily forgotton development files in an environment where multiple developers contribute.

[colin-grant-work]

I agree, but to be clear about the effect here, I've reverted back to (close) to the state before #96. Given that we now want to include many node_modules, I'd need to experiment to determine whether something like

*/
!node_modules/

would override VSCE's default behavior in which it resolves which runtime dependencies from node_modules, whereas we know that the older file is more or less correct. I'm happy to do that experimentation, but as a first cut, the reversion seemed like a safe option.

[asimgunes]

Relative to #107, it has most of what you've brought in, apart from separate esm and cjs modules. Is providing both important to your use case?

Hi @colin-grant-work, I just added ESM along with the CJS just for the best practice and general use. Only CJS is enough for my particular scenario. And happy to hear your comment if you suggest to skip ESM for now.

[jonahgraham]

@colin-grant-work / @asimgunes - I don't have strong opinions on how we get this to work, you two have much more domain knowledge and understanding than I do. I would just like to get something working asap as the deployed version of this extension on open-vsx is currently broken. Perhaps we revert #96 (and the related changes since) so we can publish a new working version and then proceed with the various ideas here?

[colin-grant-work]

@jonahgraham, I'd say in its current form, this is very close to a reversion of #96, and if you can confirm that it works on target debug cases, I think we could move forward with this and clarify the details in #107. I've tested target debugging locally, but I'm not sure under exactly what conditions the serialports come into play.

[jonahgraham]

I built this branch and can start target debug and connect the serial port.

@asimgunes can you confirm this is ok to go as is now so I can get a working version on open-vsx and then we can get #107 resolved after?

[asimgunes]

Hi @jonahgraham,

It is ok for me to move this request first since we have an active problem, I can rebase the #107 after the merge of this request.

[asimgunes]

Hi @jonahgraham , @colin-grant-work

But I forgot to mention, I still insist not renaming the MemoryBrowser bundle name here, if I am not missing anything, we do not need this renaming since the other output files are under other folders and I am not expecting any file conflict at the output.

Also fine, if you want this renaming for preventing future confusion.

[colin-grant-work]

But I forgot to mention, I still insist not renaming the MemoryBrowser bundle name here,

I've taken the renaming out of the current version - there should be no changes to MemoryServer.ts left, and you can see in the esbuild.js file that it's back to MemoryBrowser.js as its output.

[asimgunes]

I've taken the renaming out of the current version - there should be no changes to MemoryServer.ts left, and you can see in the esbuild.js file that it's back to MemoryBrowser.js as its output.

I think ts compiler should generate the MemoryBrowser under dist/memory/client/MemoryBrowser.js and esbuild should generate the it to dist/MemoryBrowser.js path. Am I missing something?

[colin-grant-work]

I think ts compiler should generate the MemoryBrowser under dist/memory/client/MemoryBrowser.js and esbuild should generate the it to dist/MemoryBrowser.js path. Am I missing something?

Yes, I agree.

[asimgunes]

Yes, I agree.

Then, do you think there could be still a conflict if not renaming?

[colin-grant-work]

@asimgunes, I'm not sure we're on the same page. I believe I've made the changes you've requested so that there are no changes in name to files that were being generated before this PR. There are now two MemoryBrowser.js files: one in dist is the bundled version that the webview code refers to; one in dist/memory/client that is the product of tsc. That's OK, if not ideal, because they're in different places, so they don't clobber each other.

Have I misunderstood your initial request, and is there a change you'd like to see to the current code?

[asimgunes]

@colin-grant-work, ok I did not realise the code change. It seems fine now. Thanks.

[colin-grant-work]

@jonahgraham, it looks like we're agreed on the current state.

[jonahgraham]

OK. I am merging this and will tag a 0.0.107 version soon that will be published overnight.