Move to ESM modules #8460

Pessimistress · 2024-01-30T00:14:32Z

Changelist

Bump ocular-dev-tools
package.json
Remove babel config, use Typescript transforms
Minor change to bundle entry points to use transformed code, see comments inline
Minor change to some import paths (relative imports must reference filename, i.e. import {add} from '../utils' => import {add} from '../utils/index') see Move to ESM modules #8460 (comment)

Tested Workflows

Bootstrap
Build
- ESM entry (import('@deck.gl/core'))
- CJS entry (require('@deck.gl/core'))
- Script bundle
Lint
Examples start-local
Test
- node
- browser-headless

TODO

The following modules do not build: arcgis, test-utils due to TypeScript errors, will address in future PRs
Migrate babel remove-glsl-comments plug-in to TS transform

examples/layer-browser/src/data-samples.js

modules/aggregation-layers/bundle.ts

felixpalmer · 2024-01-30T12:39:08Z

modules/carto/src/api/fetch-map.ts

@@ -16,7 +16,7 @@ import {
  vectorQuerySource,
  vectorTableSource,
  vectorTilesetSource
-} from '../sources';
+} from '../sources/index';


We could avoid all these 'xxxxxx/index' imports by using experimentalSpecifierResolution in tsconfig

Note that these import paths don't just affect source code, they also persist into *.d.ts files, and will break in a downstream application with TSConfig using node16(+) or nodenext. See:

ES Modules and "moduleResolution":"NodeNext" in v9 luma.gl#1861

Split TS declaration files are not compatible with nodenext module resolution developit/microbundle#1019 (comment)

Could not find a declaration file for module '@gltf-transform/core' [Node.js/Typescript] donmccurdy/glTF-Transform#824

Ideally — I think we should either go one step further and add .js extensions, like ../sources/index.js, or else take @ibgreen's suggestion from visgl/luma.gl#1861 (comment) and post-process the *.d.ts files.

.js extension is added with a custom typescript transform, hence xxx/index which is processed into xxx/index.js.

We could alternatively write all the extensions into src, I was trying to avoid too many changes.

I'd missed the custom TS transform, that sounds like a good solution to me!

One other option would be to bundle the type definitions into a single file with something like tsup or dts-buddy — no need for extensions if there are no imports! — but if the custom TS transform is working already, I'd vote for that. 👍🏻

I like the idea of bundling the d.ts files. Just like we now bundle both the ESM and commonJS code, it would be neat to bundle the types. If you are familiar with the tools, you could perhaps test it in some repo and then we can upstream to ocular?

modules/aggregation-layers/package.json

ibgreen

Apart from removing the @luma.gl/webgl re-export in scripts, these are just nits and thoughts.

ibgreen · 2024-02-05T13:27:09Z

.ocularrc.js

    format: 'umd',
    globals: {
      '@deck.gl/*': 'globalThis.deck',
      '@luma.gl/core': 'globalThis.luma',
+      '@luma.gl/engine': 'globalThis.luma',


Q: If the * notation works, might be useful for both luma and loaders? Any reason not to?

The luma and loaders global objects are to make sure that there is only one copy of a module. User access of the exported classes is just a bonus. This is also the reason why I'm not cherry picking the exports - what's exported is not determined by their usefulness, but by necessity if they are referenced anywhere in the code path. If we cherry pick, it's going to be a long list due to the sprawling interface between deck and luma, and the list has to be re-audited every time we make a change.

ibgreen · 2024-02-05T13:29:02Z

modules/arcgis/package.json

+  "module": "dist/index.js",
+  "exports": {
+    ".": {
+      "import": "./dist/index.js",


This module is still in js, see todo list.

ibgreen · 2024-02-05T13:31:47Z

modules/carto/src/api/fetch-map.ts

@@ -16,7 +16,7 @@ import {
  vectorQuerySource,
  vectorTableSource,
  vectorTilesetSource
-} from '../sources';
+} from '../sources/index';


I like the idea of bundling the d.ts files. Just like we now bundle both the ESM and commonJS code, it would be neat to bundle the types. If you are familiar with the tools, you could perhaps test it in some repo and then we can upstream to ocular?

ibgreen · 2024-02-05T13:33:52Z

modules/carto/src/index.ts

@@ -32,7 +32,7 @@ export type {
  RequestType,
  QueryParameters,
  QueryOptions
-} from './api';
+} from './api/index';


Total nit: I am not a fan of nested index files, I'd just import from the individual files. But since this is the carto module my opinion doesn't matter :)

ibgreen · 2024-02-05T13:35:44Z

modules/core/bundle/debug.ts

@@ -1,11 +1,7 @@
-import {getLoggers} from '../src/debug/loggers';
+// @ts-nocheck
+import {getLoggers} from '@deck.gl/core/debug/loggers';


Q: Do we even need this module? It only seems to repackage core stuff? Something we could drop in v9?Are we trying to replace a call to deck.registerLoggers() with an require in scripts. Is it to handle some timing condition during imports? Not a big deal, if this is still useful feel free to keep it, no need to have long discussion.

This importing from module subpaths is a bit of a yellow flag to me. It has caused me a lot of problems in the past with various build tools, so I would avoid it if possible. Maybe just export _getLoggers from the main package?

The whole point of this file is to NOT include verbose debug messages in the main bundle. https://deck.gl/docs/developer-guide/debugging#deckgl-logging

The import path is not exposed by the published package. This file is only used when building debug.min.js.

ibgreen · 2024-02-05T13:48:30Z

modules/core/src/index.ts

@@ -132,7 +139,7 @@ export type {PickingInfo, GetPickingInfoParams} from './lib/picking/pick-info';
 export type {ConstructorOf as _ConstructorOf} from './types/types';
 export type {BinaryAttribute} from './lib/attribute/attribute';
 export type {Effect, PreRenderOptions, PostRenderOptions} from './lib/effect';
-export type {PickingUniforms, ProjectUniforms} from './shaderlib';
+export type {PickingUniforms, ProjectUniforms} from './shaderlib/index';


Is this listing of /index required by the new tooling or just a personal preference?

Total nit, my preference is to avoid nested index files. There are of course exceptions, but generally to me they end up just adding more clutter and indirection.

So I guess I could argues that this is a good change as doesn't actually add index files but rather shines a light on where we are using index files.

You can no longer import from directory in ESM.
#8460 (comment)

I don't have a strong opinion about code organization, just trying to keep irrelevant changes to the minimum in this PR.

ibgreen · 2024-02-05T13:52:59Z

modules/core/src/scripting/lumagl.ts

+export * from '@luma.gl/core';
+export * from '@luma.gl/engine';
+// @ts-ignore Module '@luma.gl/core' has already exported a member named 'AccessorObject'
+export * from '@luma.gl/webgl';


We should not be exposing the webgl module anymore. If there is any particular export we feel we need that isn't visible from core, it should be solved in luma.gl

The webgl module does not work if there are multiple copies, so @deck.gl/core bundles it and exposes it under globalThis.luma. Unless you want the users of the scripting interface to always include an additional package from luma.gl, this has to stay.

Summarize an offline discussion: we should make a follow-up pass of the code base to completely remove any direct reference of @luma.gl/webgl, after which this re-export will not be necessary.

ibgreen · 2024-02-05T14:01:09Z

scripts/postinstall.sh

@@ -1,15 +1,4 @@
 #!/bin/bash

-# Add commonjs entry points to pure ESM modules. Needed by running node tests.
-npx babel node_modules/@mapbox/tiny-sdf/index.js > node_modules/@mapbox/tiny-sdf/index.cjs


I guess CJS applications using deck still need to do this?

Well that's not really within our control..

chrisgervang · 2024-02-09T17:49:51Z

babel.config.js

Is there a replacement for these widget overrides somewhere else?

Pessimistress requested review from felixpalmer, donmccurdy, chrisgervang and ibgreen January 30, 2024 00:14

felixpalmer reviewed Jan 30, 2024

View reviewed changes

felixpalmer mentioned this pull request Jan 31, 2024

Bump quadbin to 0.2.0 #8462

Merged

donmccurdy reviewed Jan 31, 2024

View reviewed changes

modules/aggregation-layers/package.json Show resolved Hide resolved

Pessimistress force-pushed the x/esm branch from f982495 to d5accde Compare February 4, 2024 19:22

Pessimistress changed the title ~~[WIP] Move to ESM modules~~ Move to ESM modules Feb 4, 2024

Pessimistress marked this pull request as ready for review February 4, 2024 19:22

ibgreen approved these changes Feb 5, 2024

View reviewed changes

Pessimistress added 2 commits February 5, 2024 14:28

wip

e7dccc4

fix lint

78bff65

Pessimistress force-pushed the x/esm branch from d5accde to 78bff65 Compare February 5, 2024 22:28

Pessimistress merged commit 940da34 into master Feb 5, 2024
2 checks passed

Pessimistress deleted the x/esm branch February 6, 2024 20:50

donmccurdy mentioned this pull request Feb 8, 2024

chore(types): Remove remaining references to /typed entrypoints #8481

Merged

Pessimistress mentioned this pull request Feb 8, 2024

[Bug] dist tests broken since introduction of ESM #8482

Closed

7 tasks

chrisgervang reviewed Feb 9, 2024

View reviewed changes

babel.config.js

Copy link

Collaborator

chrisgervang Feb 9, 2024

Choose a reason for hiding this comment

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

Is there a replacement for these widget overrides somewhere else?

chrisgervang mentioned this pull request Feb 13, 2024

Bump widget example to v9 #8492

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to ESM modules #8460

Move to ESM modules #8460

Pessimistress commented Jan 30, 2024 •

edited

felixpalmer Jan 30, 2024

donmccurdy Jan 30, 2024 •

edited

Pessimistress Jan 30, 2024

donmccurdy Jan 30, 2024 •

edited

ibgreen Feb 5, 2024

ibgreen left a comment

ibgreen Feb 5, 2024

Pessimistress Feb 5, 2024

ibgreen Feb 5, 2024

Pessimistress Feb 5, 2024

ibgreen Feb 5, 2024

ibgreen Feb 5, 2024

ibgreen Feb 5, 2024

ibgreen Feb 5, 2024

Pessimistress Feb 5, 2024

ibgreen Feb 5, 2024

Pessimistress Feb 5, 2024 •

edited

ibgreen Feb 5, 2024

Pessimistress Feb 5, 2024

Pessimistress Feb 5, 2024

ibgreen Feb 5, 2024

Pessimistress Feb 5, 2024

chrisgervang Feb 9, 2024

Move to ESM modules #8460

Move to ESM modules #8460

Conversation

Pessimistress commented Jan 30, 2024 • edited

Changelist

Tested Workflows

TODO

Choose a reason for hiding this comment

donmccurdy Jan 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy Jan 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibgreen left a 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

Pessimistress Feb 5, 2024 • edited

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

Pessimistress commented Jan 30, 2024 •

edited

donmccurdy Jan 30, 2024 •

edited

donmccurdy Jan 30, 2024 •

edited

Pessimistress Feb 5, 2024 •

edited