Update test harness #7523
Update test harness #7523
Conversation
|
Render tests are failing due to Chrome version update. Some of the visual differences are not trivial; I need more time to investigate. |
There was a problem hiding this comment.
Impressive effort!
However, two things stand out as particularly problematic
- the patches, this doesn't seem like something we should be dealing with if at all possible
- the extensive changes to the jupyter module, which I really would like to see as a separate PR. Given the low amount of maintainer bandwidth that component has, I would prefer for work there to be isolated and carefully reviewed rather than shoehorned in via a monster infrastructure PR.
| const deck = globalThis.deck || {}; | ||
|
|
||
| // Check if peer dependencies are included | ||
| if (!deck.LineLayer) { | ||
| throw new Error('@deck.gl/layers is not found'); | ||
| } | ||
|
|
||
| module.exports = Object.assign(deck, deckGLLayers); | ||
| export default Object.assign(deck, deckglAggregationLayers); |
There was a problem hiding this comment.
Maybe this comment is best resolved at a later stage by creating two separate bundle.ts scripts, but...
I think we should aim to build scripts that support the new dynamic import script syntax. And if so a default export is not what one would expect from such a "type: module" script. That module should only export its actual symbols.
Also splitting the side effect (Object.assign) from the export would be a little clearer as they are quite different.
There was a problem hiding this comment.
I can test it. The bundles are not currently used via require() anyways, but via the global window.deck object.
| @@ -27,7 +27,7 @@ | |||
| ], | |||
| "sideEffects": false, | |||
| "scripts": { | |||
| "build-debugger": "webpack ./bundle/debug.js -p -o ./debug.min.js --config ../../scripts/bundle.config.js", | |||
| "build-debugger": "webpack ./bundle/debug.ts -p -o ./debug.min.js --config ../../scripts/bundle.config.js", | |||
There was a problem hiding this comment.
Interesting, what does build-debugger do?
There was a problem hiding this comment.
modules/core/src/lib/init.ts
Outdated
| @@ -25,17 +25,19 @@ import log from '../utils/log'; | |||
| import {register} from '../debug'; | |||
| import jsonLoader from '../utils/json-loader'; | |||
|
|
|||
| const _global: any = globalThis; | |||
There was a problem hiding this comment.
This does not look right. You should be able to declare global or just use globalThis['DECK_VERSION'].
Pessimistress
force-pushed
the
x/dev-tools
branch
2 times, most recently
from
43393bb
to
79f4271
Compare
Pessimistress
force-pushed
the
x/dev-tools
branch
from
79f4271
to
b39d6cf
Compare
Pessimistress
force-pushed
the
x/dev-tools
branch
from
b39d6cf
to
5e39d82
Compare
|
|
||
| let failed = false; | ||
| // @ts-expect-error browserTestDriver_finish injected by BrowserTestDriver |
There was a problem hiding this comment.
Nit for later: Might be nice to get rid of the ts-expect-errors here?
declare global {
var browserTestDriver_fail: any; // Or maybe better function type.
...
}
With this PR:
ts-nodeinstead of@babel/register, which gives us type check and behavior more consistent with the TS specviteinstead ofwebpack. It is much faster and again better with type checks. The eventual goal is to move away from the outdated webpack 4 dependency.There is a lot happening here, see a summary below.
Change List
ocular-dev-tools,react,probe.glrequire()calls from source and tests.vitedoes not polyfill the browser environment as aggressively aswebpack)d3andtiny-sdfare oftype: 'module'and cannot be imported from commonjs under Node.ts-nodehas experimental support for esm modules, however module aliasing does not work yet. For now I'm patching these packages locally to add a cjs entry point.