Bundling via esbuild. #30

dbreese · 2023-06-26T16:22:14Z

Note:

Still not clear to me if node_modules can be bundled. I could not get it to be bundled, and without the node_modules pruned directory, the extension will not run. Documentation appears to state that VSCode will automatically install dependencies, but that does not appear to be the case.
The generated vsix file is around 26megs in size.

khawkins · 2023-06-26T16:42:18Z

.vscodeignore

+RELEASE.md
+coverage/
+.nyc_output/
+.github


We need to do this with folders like out/ and node_modules/ too. There should be a very lightweight, file-count-wise, package at the end of the line, because everything is meant to be bundled into one file.

If I remove node_modules, the generated extension does not work. Also, the out is needed because that is where our extension is compiled to, which is our entrypoint. I dont think we can discard these two dirs.

Did some more testing just to validate what I recalled from last week ;) :

If I remove out/, then the package wont be generated because it complains of a missing entry point (main in package.json defines it as /out/extension.js

If I remove node_modules/, then the generated package is only 20k, but fails to launch at runtime because:

2023-06-26 11:19:33.659 [error] Error: Cannot find module '@salesforce/lwc-dev-mobile-core/lib/common/CommonUtils' Require stack: - /Users/dbreese/.vscode/extensions/salesforce.salesforcedx-vscode-mobile-0.0.1/out/extension.js - /Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/loader.js - /Applications/Visual Studio Code.app/Contents/Resources/app/out/bootstrap-amd.js - /Applications/Visual Studio Code.app/Contents/Resources/app/out/bootstrap-fork.js

Yeah I'm basically fiddling around in my environment, seeing similar things.

There's a gap of documentation about some of the entries in your standard package.json file vs. what esbuild is / might be expecting. For example, I switched locally to have esbuild use an --outfile value of dist/main.js just to see how it went. And while it generates an all-in-one dist/main.js, it still complains at runtime about trying to use out/extension.js, which is the main entry point in package.json.

The dearth of comprehensive documentation is...frustrating. I'm going to keep poking around.

if you just change --outfile to out/extension.js it should work though so that it matches package.json's main value?

Yeah but I didn't like that because that's where we compile everything out too, and so you have to do a bunch of filtering to not include a swathe of JS files down there.

Locally, I ended up specifying --outfile as dist/main.js, and made that my main target in package.json too, and that got past that particular hurdle (and onto another one, but that's another story).

khawkins · 2023-06-26T16:48:30Z

package.json

@@ -58,7 +49,8 @@
    "test-coverage": "node ./out/test/runTest.js --coverage",
    "prettier:write": "prettier --write \"src/**/*.{ts, js}\"",
    "prettier:verify": "prettier --list-different \"src/**/*.{ts, js}\"",
-    "vscode:prepublish": "npm run pretest && npm run test"
+    "bundle:extension": "esbuild ./src/extension.ts  --bundle --outdir=out --format=cjs --target=es2020 --platform=node --external:vscode --external:@salesforce/core --external:@salesforce/lwc-dev-mobile-core --external:@salesforce/sf-plugins-core",


I'm not seeing the output file of the bundling process. You don't specify --outfile here. I think you need to?

No, I am using--outdir so it is just generated there as extension.js which includes bundled src from all of our source files.

khawkins · 2023-06-28T19:04:23Z

package.json

@@ -58,7 +50,8 @@
    "test-coverage": "node ./out/test/runTest.js --coverage",
    "prettier:write": "prettier --write \"src/**/*.{ts, js}\"",
    "prettier:verify": "prettier --list-different \"src/**/*.{ts, js}\"",
-    "vscode:prepublish": "npm run pretest && npm run test"
+    "bundle:extension": "esbuild ./src/extension.ts  --bundle --outdir=out --format=cjs --target=es2020 --platform=node --external:vscode --external:@salesforce/core --external:@salesforce/lwc-dev-mobile-core --external:@salesforce/sf-plugins-core --minify",


I'd like to see some changes here. Gonna mention some files outside of this one too.

Tack to --outfile from --outdir. The latter is if you need to call multiple targets within a package, which we don't.

In my experience, the only two --external: directives we need are vscode and @oclif/core: the former because it won't get resolved, and the latter because it tries to access a relative-pathed package.json that doesn't otherwise exist in the right place, when the package is inlined into extension.js. The other --external: directives can go away, as we're including all of node_modules/—or rather, not excluding it—in .vscodeignore.

Speaking of .vscodeignore: we should ignore all of out/, and bang-include (whitelist) !out/external.js. We don't need the other artifacts in there—esbuild will incorporate them into the external.js end product. And the leftover code could negatively impact the bundle at runtime in unforeseen ways.

Cool, let me try and update this.

I also updated the build so that it runs a clean, so the out directory SHOULD BE empty. However, perhaps vscode instance could do a concurrent compile and shove *.js files in there too. I'll try the !out/extension.js approach.

If I change the --external:* paths to just have vscode and @oclif/core, the result extension.js bundle is 2.4megs. Leaving in what I have, it is only 10k. I am trying to figure out what exactly it is bundling.

If I remove the --external:* options (except for vscode and @oclif/core, and also ignore node_modules/*, then the extension does not work at all. It will install, but immediately shows error when launched.

I am still not sure WHY removing these --external:* options makes the out/extension.js file be so much larger - presumably it is trying to bundle at least PARTS of the dependencies?

It does appear that it is attempting to bundle some things in node_modules: grep ^// extension.js shows the file paths such as:

// node_modules/@salesforce/ts-types/lib/narrowing/is.js // node_modules/@salesforce/ts-types/lib/narrowing/as.js // node_modules/@salesforce/ts-types/lib/errors.js // node_modules/@salesforce/ts-types/lib/narrowing/to.js // node_modules/@salesforce/ts-types/lib/narrowing/assert.js // node_modules/@salesforce/ts-types/lib/narrowing/coerce.js // node_modules/@salesforce/ts-types/lib/narrowing/ensure.js // node_modules/@salesforce/ts-types/lib/narrowing/has.js ...

and also ignore node_modules/*

We can't ignore node_modules/ in this iteration. That requires more fine tuning of --external: directives than we're going to bite off in V1, to ensure that all of the dependencies of any --external: directives are also external. I started playing with that, but it's painful and probably needs automation to be maintainable.

But in the absence of adding any .vscodeignore directives for node_modules, you should be able to attenuate your --external: directives to the two I listed.

I am still not sure WHY removing these --external:* options makes the out/extension.js file be so much larger - presumably it is trying to bundle at least PARTS of the dependencies?

Correct. When packages are not otherwise called out with --external: directives, esbuild interrogates and inlines all of those packages and their dependencies into extension.js. That's why that file gets bigger—otherwise, --external: packages just continue to live on their own in node_modules/, and extension.js refers to them in a more normal require type of way in those cases.

I also updated the build so that it runs a clean, so the out directory SHOULD BE empty. However, perhaps vscode instance could do a concurrent compile and shove *.js files in there too. I'll try the !out/extension.js approach.

Oh that's a good point: esbuild may only generate extension.js, at which point cleaning beforehand effectively gets us to the same place. I was thinking esbuild would generate the same output as e.g. tsc does for us in development.

khawkins · 2023-06-28T19:09:19Z

package.json

@@ -58,7 +50,8 @@
    "test-coverage": "node ./out/test/runTest.js --coverage",
    "prettier:write": "prettier --write \"src/**/*.{ts, js}\"",
    "prettier:verify": "prettier --list-different \"src/**/*.{ts, js}\"",
-    "vscode:prepublish": "npm run pretest && npm run test"
+    "bundle:extension": "esbuild ./src/extension.ts  --bundle --outdir=out --format=cjs --target=es2020 --platform=node --external:vscode --external:@salesforce/core --external:@salesforce/lwc-dev-mobile-core --external:@salesforce/sf-plugins-core --minify",
+    "vscode:prepublish": "npm run clean && npm run bundle:extension && npm prune --omit=dev"


Shouldn't npm prune --omit=dev happen before the bundle:extension step? Or otherwise if that doesn't work with stuff put together all in this one prepublish step, perhaps this step should be broken out to first build, then omit, and then bundle.

Otherwise, I don't see the prune step doing anything for us here.

I thought the same thing yesterday, but i believe what is happening is prepublish runs, then bundling occurs, THEN the packaging occurs. If I change these around, the prune will remove all dev-deps and then esbuild wont even be found. I also tested it yesterday by adding a && ls -la to the end of bundle:extension and no *.vsix file exists after esbuild executes.

Yeah, I figured simply changing the order would probably break things. I think you'd need to do a build step, then an omit step, and then a bundle step. Right now the "build and bundle" is all merged together in one command, so it's not possible.

It's not a big deal though, either way. I'm okay leaving it as is for this release.

khawkins · 2023-06-28T19:12:11Z

package.json

@@ -58,7 +50,8 @@
    "test-coverage": "node ./out/test/runTest.js --coverage",
    "prettier:write": "prettier --write \"src/**/*.{ts, js}\"",
    "prettier:verify": "prettier --list-different \"src/**/*.{ts, js}\"",
-    "vscode:prepublish": "npm run pretest && npm run test"
+    "bundle:extension": "esbuild ./src/extension.ts  --bundle --outdir=out --format=cjs --target=es2020 --platform=node --external:vscode --external:@salesforce/core --external:@salesforce/lwc-dev-mobile-core --external:@salesforce/sf-plugins-core --minify",


Your script creation is slightly different than the doc example, but piecing it together: should we have the --sourcemap option in here too?

Ah, let me try adding it. I have gone through so many iterations of the esbuild commands, not sure how I arrived at this one, but other than the --sourcemap, looks close unless I'm missing something?

It looks pretty much the same as the example, otherwise. I like the way the example in the docs is split out, because it also provides a target for being able to debug. But we can also cover that split-out in our next iteration of bundling to come. I'm okay with it as we've coded it today. Presumably --sourcemap won't negatively impact what we have here.

Oh I just noticed I looked at it wrong! In the example configuration, vscode:prepublish calls the esbuild-base script, which does not specify --sourcemap. The esbuild script—which is not otherwise called in the publishing workflow—calls esbuild-base, but with the --sourcemap argument added on.

So presumably --sourcemap not necessary for actual publishing, and is probably more useful in local use cases?

I understand it is more for generating the mapping from minified to debug code, similar to a dsym on iOS or mapping.txt for Android proguard obfuscation? It is being generated, but not bundled in the VSIX currently.

Exactly: it's meant to tie back to code in a development environment, to facilitate stepping through minified code, or JS code transpiled from TypeScript, etc. It's not required for runtime functionality, just for debugging it. So they don't have any real purpose in your final bundle. 👍

khawkins

That's all I had. We can discuss the changes further if you like.

It does create the sourcemap in out/extension.js.map but does not include it in the vsix.

dbreese · 2023-06-28T21:46:33Z

Cool, thanks for the feedback @khawkins ! Sorry this is taking so many iterations to get done!

khawkins · 2023-06-28T21:47:41Z

Cool, thanks for the feedback @khawkins ! Sorry this is taking so many iterations to get done!

Haha no worries. This has been educational for me too. 🥲 😜

khawkins

Another suggestion I have that would fit well with this PR—but could be a follow-on: we should probably add @vscode/vsce as a dev dependency to the project. Eventually/soon we're likely going to want to (semi-)automate creating VSIX packages, and that's going to be the easiest way to ensure that the packager is accessible in CI, similar to adding esbuild as we've done.

khawkins · 2023-06-29T00:05:55Z

Another suggestion I have that would fit well with this PR—but could be a follow-on: we should probably add @vscode/vsce as a dev dependency to the project. Eventually/soon we're likely going to want to (semi-)automate creating VSIX packages, and that's going to be the easiest way to ensure that the packager is accessible in CI, similar to adding esbuild as we've done.

On second thought, let's just leave that for the story we have to automate publishing. It fits right into the requirements for that story. Disregard.

khawkins

This is working for me locally! I think we can iterate to make improvements as we go. One thing I will want to revisit when we get deeper into packaging, is not leaving the project in an undevelop-able state after creating the package. Doing the npm prune means that you basically have to do an npm install to start working again, post-package-creation. But that problem should go away when we start effectively "white-listing" the contents of our package.

Thanks for getting this across the line!

dbreese · 2023-06-29T13:22:35Z

Awesome, the npm install after packaging is driving me nuts to. It is caused by the npm prune. Will be glad to iterate on next steps, and I like the idea of adding @vscode/vsce to dev deps.

dbreese added 3 commits June 26, 2023 07:55

Initial pass at getting bundling working with esbuild.

9b7a6ea

WIP - removed --minifiy from esbuild cmd.

221a14b

Rebasing and updating location of html files.

921cf09

dbreese requested a review from a team as a code owner June 26, 2023 16:22

khawkins reviewed Jun 26, 2023

View reviewed changes

dbreese added 4 commits June 26, 2023 11:34

Adding --minify

1df8a11

Updating to remove internal artifacts.

02bdaf6

Fixing compile issue by backing out updates to package-lock.

47073aa

Added clean step.

b60ef7e

khawkins reviewed Jun 28, 2023

View reviewed changes

khawkins requested changes Jun 28, 2023

View reviewed changes

Adding --sourcemap and ignores.

162392f

It does create the sourcemap in out/extension.js.map but does not include it in the vsix.

khawkins reviewed Jun 28, 2023

View reviewed changes

khawkins approved these changes Jun 29, 2023

View reviewed changes

dbreese merged commit fc6a79d into main Jun 29, 2023
16 checks passed

dbreese deleted the bundling_noscripts branch June 29, 2023 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bundling via esbuild. #30

Bundling via esbuild. #30

dbreese commented Jun 26, 2023

khawkins Jun 26, 2023

dbreese Jun 26, 2023

dbreese Jun 26, 2023

khawkins Jun 26, 2023

dbreese Jun 26, 2023

khawkins Jun 26, 2023

khawkins Jun 26, 2023

dbreese Jun 26, 2023

khawkins Jun 28, 2023

dbreese Jun 28, 2023

dbreese Jun 28, 2023

dbreese Jun 28, 2023

dbreese Jun 28, 2023

dbreese Jun 28, 2023

khawkins Jun 28, 2023

khawkins Jun 28, 2023

khawkins Jun 28, 2023

dbreese Jun 28, 2023

khawkins Jun 28, 2023

khawkins Jun 28, 2023

dbreese Jun 28, 2023

khawkins Jun 28, 2023

khawkins Jun 29, 2023

dbreese Jun 29, 2023

khawkins Jun 29, 2023

khawkins left a comment

dbreese commented Jun 28, 2023

khawkins commented Jun 28, 2023

khawkins left a comment

khawkins commented Jun 29, 2023

khawkins left a comment

dbreese commented Jun 29, 2023

Bundling via esbuild. #30

Bundling via esbuild. #30

Conversation

dbreese commented Jun 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khawkins left a comment

Choose a reason for hiding this comment

dbreese commented Jun 28, 2023

khawkins commented Jun 28, 2023

khawkins left a comment

Choose a reason for hiding this comment

khawkins commented Jun 29, 2023

khawkins left a comment

Choose a reason for hiding this comment

dbreese commented Jun 29, 2023