Switch bundler to esbuild #7546
Conversation
There was a problem hiding this comment.
Great to see esbuild and bundles with proper exports!
The setup was and is fairly complicated. A short writeup could be helpful, either as a separate doc or more text in perhaps the esbuild config files.
In fact, having a generic bundle building solution in dev tools would be powerful, then we could apply it in all our repos and get consistent scripts across visgl. This looks fairly reusable, but perhaps it is still a lot of work to break it out?
As to comments:
- Avoiding the .. imports between module directories would be desirable, but maybe it can't be helped.
As follow up.
- It would be great to start generating both .cjs and .mjs bundles, the latter would support dynamic imports.
- (most CDN's probably don't set the right headers on non-
.jsfiles so we probably can't use those extensions when importing from e.g. unpkg, but we could offer .js variants).
| @@ -1,8 +1,10 @@ | |||
| import {getLoggers} from '../src/debug/loggers'; | |||
There was a problem hiding this comment.
Total nit: rename this directory to modules/core/bundles ?
modules/core/package.json
Outdated
| @@ -27,7 +27,7 @@ | |||
| ], | |||
| "sideEffects": false, | |||
| "scripts": { | |||
| "build-debugger": "node ../../scripts/bundle.mjs ./bundle/debug.ts --output=./debug.min.js", | |||
| "build-debugger": "node ../../scripts/bundle.mjs ./bundle/debug.ts --output=./debug.min.js --globalName=deck.debug", | |||
There was a problem hiding this comment.
Total nit: It would have been a bit easier on the eyes if that script was called build-bundle or even bundler rather than just bundle. There are a lot of bundle mentions so I have to pause to make sense of it.
modules/arcgis/bundle.ts
Outdated
| @@ -1,10 +1,3 @@ | |||
| import * as ArcGISUtils from './src/load-modules'; | |||
| export * from '../layers/bundle/peer-dependency'; | |||
There was a problem hiding this comment.
Importing across module directories is very surprising to the maintainer and should really be avoided. If we need something like that, perhaps put it in a special place rather than just in modules/layers/bundle ???
There was a problem hiding this comment.
This is to work around the following issue:
import {Layer} from '@deck.gl/core'; // In bundles resolved to `globalThis.deck`
if (!Layer) {
throw new Error('please include @deck.gl/core bundle');
}
export * from '../src';When it is executed, all imports are performed before code evaluation. So instead of hitting our dependency check, import fails first with an unhelpful error (a is undefined or super expression must either be null or a function). The solution for this is to put the dependency check in a separate file, and by importing said file first, the code is executed before the next import.
Do you have a suggestion for "a special place"?
| @@ -0,0 +1,11 @@ | |||
| // / This file should be included in another bundle if @deck.gl/core is expected as a peer dependency | |||
| @@ -19,10 +19,10 @@ | |||
| // THE SOFTWARE. | |||
|
|
|||
| import test from 'tape-catch'; | |||
| import deckgl from 'deck.gl/../bundle'; | |||
| import * as deckgl from 'deck.gl/../bundle'; | |||
There was a problem hiding this comment.
These imports look really weird? Assume the bundle is in the root directory? Can we place the bundle in the dist folder so we don't have to use ..? If we want to avoid breaking, we can add a copy of the bundle there?
| const loggers = getLoggers(deck.log); | ||
| deck._registerLoggers(loggers); | ||
|
|
||
| export default deck; | ||
| export {loggers}; |
There was a problem hiding this comment.
... Great to see us moving away from default exports.
| @@ -28,7 +28,7 @@ import Tooltip from './tooltip'; | |||
| import log from '../utils/log'; | |||
| import {deepEqual} from '../utils/deep-equal'; | |||
| import typedArrayManager from '../utils/typed-array-manager'; | |||
| import deckGlobal from './init'; | |||
| import {VERSION} from './init'; | |||
There was a problem hiding this comment.
Nit: Moving this probably makes the initialization code run later, but it is presumably OK in this case. But one has to be careful with such global registration stuff, deck global initialization and loader registration...
Part of the effort to remove webpack 4 from dev dependencies.
Change List
sideEffects: false, this leads to an esbuild error)