Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move to ESM modules #8460

Merged
merged 2 commits into from
Feb 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion .eslintrc.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const {getESLintConfig} = require('ocular-dev-tools/configuration');
module.exports = getESLintConfig({
react: '18.0.0',
overrides: {
parser: '',
parserOptions: {
project: ['./tsconfig.json'],
ecmaVersion: 2020
Expand Down Expand Up @@ -41,7 +42,7 @@ module.exports = getESLintConfig({

overrides: [
{
files: ['**/*.ts', '**/*.tsx', '**/*.d.ts'],
files: ['**/*.js', '**/*.ts', '**/*.tsx', '**/*.d.ts'],
rules: {
indent: 0,
// For parquet module
Expand Down
33 changes: 11 additions & 22 deletions .ocularrc.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
const {resolve, join} = require('path');
/** @typedef {import('ocular-dev-tools').OcularConfig} OcularConfig */
import {dirname, join} from 'path';
import {fileURLToPath} from 'url';

const packageRoot = resolve(__dirname);
const nodeModules = join(packageRoot, 'node_modules');
const packageRoot = dirname(fileURLToPath(import.meta.url));
const lumaModules = join(packageRoot, '../luma.gl/modules');

const LUMA_ALIASES_LOCAL = {
Expand All @@ -16,20 +17,25 @@ const LUMA_ALIASES_LOCAL = {

const useLocalLuma = false;

/** @type {OcularConfig} */
const config = {
lint: {
paths: ['modules', 'test']
// paths: ['modules', 'test', 'examples', 'website']
},

babel: false,

bundle: {
globalName: 'deck',
externals: ['h3-js'],
target: ['supports webgl', 'not dead'],
target: ['chrome110', 'firefox110', 'safari15'],
format: 'umd',
globals: {
'@deck.gl/*': 'globalThis.deck',
'@luma.gl/core': 'globalThis.luma',
'@luma.gl/engine': 'globalThis.luma',
'@luma.gl/webgl': 'globalThis.luma',
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

'@loaders.gl/core': 'globalThis.loaders',
'h3-js': 'globalThis.h3 || {}'
}
Expand All @@ -43,23 +49,6 @@ const config = {
'deck.gl-test': join(packageRoot, './test')
},

// These packages are of type: 'module' and do not work in node tests when compiled to commonjs
// ts-node has experimental ESM support, but there's no reliable module aliasing
// See https://github.com/dividab/tsconfig-paths/issues/122
nodeAliases: {
'@jupyter-widgets/base': `${packageRoot}/test/modules/jupyter-widget/dummy-jupyter-widgets-base.ts`,
'@mapbox/tiny-sdf': `${nodeModules}/@mapbox/tiny-sdf/index.cjs`,
'd3-array': `${nodeModules}/d3-array/dist/d3-array.cjs`,
'd3-color': `${nodeModules}/d3-color/dist/d3-color.cjs`,
'd3-format': `${nodeModules}/d3-format/dist/d3-format.cjs`,
'd3-interpolate': `${nodeModules}/d3-interpolate/dist/d3-interpolate.cjs`,
'd3-scale': `${nodeModules}/d3-scale/dist/d3-scale.cjs`,
'd3-time': `${nodeModules}/d3-time/dist/d3-time.cjs`,
'd3-time-format': `${nodeModules}/d3-time-format/dist/d3-time-format.cjs`,
'mapbox-gl': `${packageRoot}/test/modules/jupyter-widget/dummy-mapbox-gl.ts`,
'react-map-gl/dist/esm/mapbox/mapbox': `${nodeModules}/react-map-gl/dist/es5/mapbox/mapbox.js`
},

browserTest: {
server: {wait: 5000}
},
Expand All @@ -77,4 +66,4 @@ if (useLocalLuma) {
Object.assign(config.aliases, LUMA_ALIASES_LOCAL);
}

module.exports = config;
export default config;
26 changes: 0 additions & 26 deletions babel.config.js
Copy link
Collaborator

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?

This file was deleted.

1 change: 1 addition & 0 deletions examples/layer-browser/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"version": "0.0.0",
"private": true,
"license": "MIT",
"type": "module",
"scripts": {
"start-local": "vite --config ../vite.config.local.mjs"
},
Expand Down
30 changes: 15 additions & 15 deletions examples/layer-browser/src/data-samples.js
Original file line number Diff line number Diff line change
@@ -1,23 +1,23 @@
import {Vector3} from '@math.gl/core';

/* load data samples for display */
import allPoints from '../data/sf.bike.parking.json';
import allPoints from '../data/sf.bike.parking.json' assert { type: 'json' };
import {pointGrid} from './utils';
import {pointsToWorldGrid} from './utils/grid-aggregator';
import {default as meterTrajectorySmall} from '../data/meter-trajectory-small.json';

import {default as choropleths} from '../data/sf.zip.geo.json';
export {default as geojson} from '../data/sample.geo.json';
export {default as geojsonHole} from '../data/hole.geo.json';
export {default as geojsonLarge} from '../data/canada.geo.json';
export {default as hexagons} from '../data/hexagons.json';
export {default as routes} from '../data/sfmta.routes.json';
export {default as trips} from '../data/trips.json';
export {default as SFTrips} from '../data/sf.trips';
export {default as iconAtlas} from '../data/icon-atlas.json';
export {default as quadkeys} from '../data/sf.quadkeys.json';
export {default as s2cells} from '../data/sf.s2cells.json';
export {default as geohashes} from '../data/sf.geohashes.json';
import {default as meterTrajectorySmall} from '../data/meter-trajectory-small.json' assert { type: 'json' };

import {default as choropleths} from '../data/sf.zip.geo.json' assert { type: 'json' };
felixpalmer marked this conversation as resolved.
Show resolved Hide resolved
export {default as geojson} from '../data/sample.geo.json' assert { type: 'json' };
export {default as geojsonHole} from '../data/hole.geo.json' assert { type: 'json' };
export {default as geojsonLarge} from '../data/canada.geo.json' assert { type: 'json' };
export {default as hexagons} from '../data/hexagons.json' assert { type: 'json' };
export {default as routes} from '../data/sfmta.routes.json' assert { type: 'json' };
export {default as trips} from '../data/trips.json' assert { type: 'json' };
export {default as SFTrips} from '../data/sf.trips.json' assert { type: 'json' };
export {default as iconAtlas} from '../data/icon-atlas.json' assert { type: 'json' };
export {default as quadkeys} from '../data/sf.quadkeys.json' assert { type: 'json' };
export {default as s2cells} from '../data/sf.s2cells.json' assert { type: 'json' };
export {default as geohashes} from '../data/sf.geohashes.json' assert { type: 'json' };
export {choropleths};

export const positionOrigin = [-122.42694203247012, 37.751537058389985];
Expand Down
6 changes: 5 additions & 1 deletion modules/aggregation-layers/bundle.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
export * from '../layers/bundle/peer-dependency';

export * from './src';
// Import from package name instead of relative path
// This will be resolved to src or dist by esbuild depending on bundle settings
// dist has TS transformers applied
/* eslint-disable import/no-extraneous-dependencies */
export * from '@deck.gl/aggregation-layers';
felixpalmer marked this conversation as resolved.
Show resolved Hide resolved
15 changes: 11 additions & 4 deletions modules/aggregation-layers/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"name": "@deck.gl/aggregation-layers",
"description": "deck.gl layers that aggregate the input data into alternative representations",
"license": "MIT",
"type": "module",
"version": "9.0.0-beta.2",
"publishConfig": {
"access": "public"
Expand All @@ -16,13 +17,19 @@
"type": "git",
"url": "https://github.com/visgl/deck.gl.git"
},
"main": "dist/es5/index.js",
"module": "dist/esm/index.js",
"types": "typed/index.d.ts",
"types": "dist/index.d.ts",
"main": "dist/index.cjs",
"module": "dist/index.js",
"exports": {
".": {
"types": "./dist/index.d.ts",
"import": "./dist/index.js",
"require": "./dist/index.cjs"
}
donmccurdy marked this conversation as resolved.
Show resolved Hide resolved
},
"files": [
"dist",
"src",
"typed",
"dist.min.js"
],
"sideEffects": false,
Expand Down
2 changes: 1 addition & 1 deletion modules/aggregation-layers/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"compilerOptions": {
"composite": true,
"rootDir": "src",
"outDir": "typed"
"outDir": "dist"
},
"references": [
{"path": "../core"},
Expand Down
12 changes: 9 additions & 3 deletions modules/arcgis/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"name": "@deck.gl/arcgis",
"description": "Use deck.gl as a custom ArcGIS API for JavaScript layer",
"license": "MIT",
"type": "module",
"version": "9.0.0-beta.2",
"publishConfig": {
"access": "public"
Expand All @@ -16,12 +17,17 @@
"type": "git",
"url": "https://github.com/visgl/deck.gl.git"
},
"main": "dist/es5/index.js",
"module": "dist/esm/index.js",
"main": "dist/index.cjs",
"module": "dist/index.js",
"exports": {
".": {
"import": "./dist/index.js",
Copy link
Collaborator

Choose a reason for hiding this comment

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

types?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This module is still in js, see todo list.

"require": "./dist/index.cjs"
}
},
"files": [
"dist",
"src",
"typed",
"dist.min.js"
],
"sideEffects": false,
Expand Down
2 changes: 1 addition & 1 deletion modules/arcgis/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"compilerOptions": {
"composite": true,
"rootDir": "src",
"outDir": "typed"
"outDir": "dist"
},
"references": [
{"path": "../core"}
Expand Down
6 changes: 5 additions & 1 deletion modules/carto/bundle.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import * as CartoUtils from './src';
// Import from package name instead of relative path
// This will be resolved to src or dist by esbuild depending on bundle settings
// dist has TS transformers applied
/* eslint-disable import/no-extraneous-dependencies */
import * as CartoUtils from '@deck.gl/carto';

export * from '../layers/bundle/peer-dependency';

Expand Down
15 changes: 11 additions & 4 deletions modules/carto/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"name": "@deck.gl/carto",
"description": "CARTO official integration with Deck.gl. Build geospatial applications using CARTO and Deck.gl.",
"license": "MIT",
"type": "module",
"version": "9.0.0-beta.2",
"publishConfig": {
"access": "public"
Expand All @@ -18,13 +19,19 @@
"type": "git",
"url": "https://github.com/visgl/deck.gl.git"
},
"main": "dist/es5/index.js",
"module": "dist/esm/index.js",
"types": "typed/index.d.ts",
"types": "dist/index.d.ts",
"main": "dist/index.cjs",
"module": "dist/index.js",
"exports": {
".": {
"types": "./dist/index.d.ts",
"import": "./dist/index.js",
"require": "./dist/index.cjs"
}
},
"files": [
"dist",
"src",
"typed",
"dist.min.js"
],
"sideEffects": false,
Expand Down
2 changes: 1 addition & 1 deletion modules/carto/src/api/fetch-map.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
vectorQuerySource,
vectorTableSource,
vectorTilesetSource
} from '../sources';
} from '../sources/index';
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

.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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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. 👍🏻

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • 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?

import {parseMap} from './parse-map';
import {requestWithParameters} from './request-with-parameters';
import {assert} from '../utils';
Expand Down
2 changes: 1 addition & 1 deletion modules/carto/src/api/query.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {SOURCE_DEFAULTS} from '../sources';
import {SOURCE_DEFAULTS} from '../sources/index';
import type {SourceOptions, QuerySourceOptions, QueryResult} from '../sources/types';
import {buildQueryUrl} from './endpoints';
import {requestWithParameters} from './request-with-parameters';
Expand Down
8 changes: 4 additions & 4 deletions modules/carto/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export {default as BASEMAP} from './basemap';
export {default as colorBins} from './style/color-bins-style';
export {default as colorCategories} from './style/color-categories-style';
export {default as colorContinuous} from './style/color-continuous-style';
export {CartoAPIError, fetchMap, query} from './api';
export {CartoAPIError, fetchMap, query} from './api/index';
export type {
APIErrorContext,
FetchMapOptions,
Expand All @@ -32,7 +32,7 @@ export type {
RequestType,
QueryParameters,
QueryOptions
} from './api';
} from './api/index';
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 :)


import {
boundaryQuerySource,
Expand All @@ -48,7 +48,7 @@ import {
vectorTableSource,
vectorTilesetSource,
SOURCE_DEFAULTS
} from './sources';
} from './sources/index';

const CARTO_SOURCES = {
boundaryQuerySource,
Expand Down Expand Up @@ -98,4 +98,4 @@ export type {
VectorTilesetSourceOptions,
GeojsonResult,
JsonResult
} from './sources';
} from './sources/index';
2 changes: 1 addition & 1 deletion modules/carto/src/sources/boundary-query-source.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {QueryParameters} from '../api';
import {QueryParameters} from '../api/index';
import {baseSource} from './base-source';
import type {SourceOptions, TilejsonResult} from './types';

Expand Down
2 changes: 1 addition & 1 deletion modules/carto/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"compilerOptions": {
"composite": true,
"rootDir": "src",
"outDir": "typed"
"outDir": "dist"
},
"references": [
{"path": "../core"},
Expand Down
12 changes: 4 additions & 8 deletions modules/core/bundle/debug.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
import {getLoggers} from '../src/debug/loggers';
// @ts-nocheck
import {getLoggers} from '@deck.gl/core/debug/loggers';

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

/* global deck */
declare global {
const deck: any;
}

const loggers = getLoggers(deck.log);
deck._registerLoggers(loggers);
const loggers = getLoggers(globalThis.deck.log);
globalThis.deck._registerLoggers(loggers);

export {loggers};
17 changes: 11 additions & 6 deletions modules/core/bundle/index.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
// We use `require` here because luma and deck core must be imported before `global`
import * as LumaGL from '../src/scripting/lumagl';
import * as LoadersGL from '../src/scripting/loadersgl';
// @ts-nocheck
// Luma and deck core must be imported before `global`
/* eslint-disable import/no-extraneous-dependencies */
import * as LumaGL from '@deck.gl/core/scripting/lumagl';
import * as LoadersGL from '@deck.gl/core/scripting/loadersgl';

globalThis.luma = globalThis.luma || {};
globalThis.loaders = globalThis.loaders || {};

Object.assign(globalThis.luma, LumaGL);
Object.assign(globalThis.loaders, LoadersGL);

export * from '../src';
export {register as _registerLoggers} from '../src/debug';
// Import from package name instead of relative path
// This will be resolved to src or dist by esbuild depending on bundle settings
// dist has TS transformers applied
export * from '@deck.gl/core';
export {register as _registerLoggers} from '@deck.gl/core/debug';

export {default as DeckGL} from '../src/scripting/deckgl';
export {default as DeckGL} from '@deck.gl/core/scripting/deckgl';