-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Move to ESM modules #8460
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
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
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. types? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ import { | |
vectorQuerySource, | ||
vectorTableSource, | ||
vectorTilesetSource | ||
} from '../sources'; | ||
} from '../sources/index'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could avoid all these There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Ideally — I think we should either go one step further and add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. .js extension is added with a custom typescript transform, hence We could alternatively write all the extensions into src, I was trying to avoid too many changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 👍🏻 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
import {parseMap} from './parse-map'; | ||
import {requestWithParameters} from './request-with-parameters'; | ||
import {assert} from '../utils'; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -32,7 +32,7 @@ export type { | |
RequestType, | ||
QueryParameters, | ||
QueryOptions | ||
} from './api'; | ||
} from './api/index'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
@@ -48,7 +48,7 @@ import { | |
vectorTableSource, | ||
vectorTilesetSource, | ||
SOURCE_DEFAULTS | ||
} from './sources'; | ||
} from './sources/index'; | ||
|
||
const CARTO_SOURCES = { | ||
boundaryQuerySource, | ||
|
@@ -98,4 +98,4 @@ export type { | |
VectorTilesetSourceOptions, | ||
GeojsonResult, | ||
JsonResult | ||
} from './sources'; | ||
} from './sources/index'; |
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'; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}; |
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'; |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.