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

luma v9.0.0 alpha.5 #7451

Merged
merged 10 commits into from Dec 1, 2022
Merged

luma v9.0.0 alpha.5 #7451

merged 10 commits into from Dec 1, 2022

Conversation

ibgreen
Copy link
Collaborator

@ibgreen ibgreen commented Nov 23, 2022

For #7457

Background

Change List

@ibgreen
Copy link
Collaborator Author

ibgreen commented Nov 23, 2022

@Pessimistress Having problems even starting tests... Can you verify if

  • node tests can run (start)?
  • browser tests can run (start)?
  • development can be done on Node 16? Node 14 installs seems to have issues with M1 Macs.

@ibgreen
Copy link
Collaborator Author

ibgreen commented Nov 23, 2022

Also, JS build log shows lots of type issues that don't show up in luma.gl - wonder if there are different typescript settings that trigger it but haven't found it.

@ibgreen
Copy link
Collaborator Author

ibgreen commented Nov 23, 2022

Also, JS build log shows lots of type issues that don't show up in luma.gl - wonder if there are different typescript settings that trigger it but haven't found it.

The reason seems to be that deck.gl sets "strict: true" in tsconfig.json. Doing the same in luma.gl generates about 600 errors. I can start fixing them there while we disable strict here temporarily.

@felixpalmer
Copy link
Collaborator

I've pulled this branch and tried running the tests. I'm on a M1 Mac and using node 14.18.1 (installed using nvm without issues). The tests (yarn test fast) are not running for me, with the following error:

Running node tests...
deck.gl/node_modules/@luma.gl/test-utils/dist/index.js:1
export { default as SnapshotTestRunner } from './snapshot-test-runner';
^^^^^^

SyntaxError: Unexpected token 'export'

Comparing node_modules/@luma.gl/test-utils/package.json between v8 & v9, in v9 both the main and module entries point at dist/index.js (an ESM module) while in v8 main points to dist/es5/index.js

@ibgreen
Copy link
Collaborator Author

ibgreen commented Nov 28, 2022

OK. I can certainly see us vigorously debating the merits and disadvantages of ESM modules, but trying to dodge that question for a moment to avoid a complete overload of required changes on the luma side.

It was a while ago, but as far as I recall this was not an issue when I did a test integration six months back. It could be that recent changes in node.js tooling on the deck.gl side now makes deck requires a more heavily transpiled build?

It does seem that yarn test browser does not break on the exports, however I get a crash on the PipelineFactory there. That seems debuggable. Maybe we can start with browser tests?

@ibgreen
Copy link
Collaborator Author

ibgreen commented Nov 29, 2022

run test browser gets through over a thousand tests before failing, but then there are a lot of failures seemingly related to hook injections.

Screen Shot 2022-11-29 at 6 21 19 AM

It would be good to get some debug help with this as I find this system a bit tricky to follow. I did overhaul the luma.gl shadertools module in multiple passes to add strict types, so it is possible that I have introduced a bug on the luma side, though the shader injection tests do pass.

Looks like the hooks are "declared" globally here: https://github.com/visgl/deck.gl/blob/luma-v9.0.0-alpha.5/modules/core/src/shaderlib/index.ts#L44. But somehow actual values for those hooks fail to inject.

It could help to understand how these hook definitions are supplied and how they flow through to the final shader. Through the shader modules that a shader picks?

@felixpalmer @Pessimistress

@ibgreen
Copy link
Collaborator Author

ibgreen commented Nov 29, 2022

I am also not able to change test files to typescript, is that intentional?

/Users/ibgreen/code/deck.gl/test/modules/core/effects/lighting-effect.spec.ts
  0:0  error  Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser.
The file does not match your project config: test/modules/core/effects/lighting-effect.spec.ts.
The file must be included in at least one of the projects provided

@ibgreen
Copy link
Collaborator Author

ibgreen commented Nov 29, 2022

Created v9 trackerL #7457.

Some of these tasks are very generic and could be done in parallel.

@ibgreen
Copy link
Collaborator Author

ibgreen commented Nov 29, 2022

Some automated formatter is stripping out types casts. Those cases were probably added to make strict mode work, and perhaps they are being stripped now that I disable strict mode. On the other hand those casts and exclamation marks are potentially hiding problems.

@ibgreen
Copy link
Collaborator Author

ibgreen commented Dec 1, 2022

Looks like some examples might be starting to work. Landing as baseline.

@ibgreen ibgreen merged commit d965f0f into 9.0-dev Dec 1, 2022
@ibgreen ibgreen deleted the luma-v9.0.0-alpha.5 branch December 1, 2022 20:10
ibgreen added a commit that referenced this pull request Dec 30, 2022
Pessimistress pushed a commit that referenced this pull request Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants