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

Introduce platform abstraction layer #422

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Conversation

wouterlucas
Copy link
Contributor

This is to abstract away Web / Browser specific APIs in a Platform layer. By default Lightning 3 will load the Web platform as that's our most common usecase.

However this PR opens up the possibility for other platforms/runtimes to run Lightning 3, just like Lightning 2 has.

Lightning 3 now exposes a CorePlatform and CoreGlContext on which the platform specific APIs are abstracted. By default the project comes with a WebPlatform and a WebGlContext implementation for browsers.

if (settings.platform) {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
platform = new settings.platform() as WebPlatform;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we allow types of CorePlatform as setting, I don't think we need to type cast this to WebPlatform. Related to this; it should be safe to change the following typing from glw: CoreGlContext | WebGlContext to glw: CoreGlContext

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well that's what I'm struggling with, the instanceof only works in runtime and not during TS checks. Ideally I want to say well I don't care what you stick into the platform, as long as its an instance of CorePlatform.

FYI had some convo's with the ION group and we might not need the GL abstraction afterall, so might limit this to web platform api stuff like create canvas only. But waiting for one more check, if they come back with we don't need to abstract the GL layer we can go back to the wrapper as it was and only do the WebPlatform portion.

// @ts-ignore
platform = new settings.platform() as WebPlatform;
} else {
platform = new WebPlatform();

Choose a reason for hiding this comment

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

Won't this mean we'll always have to include the WebPlatform on the bundle even though we might not use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Won't this mean we'll always have to include the WebPlatform on the bundle even though we might not use it?

yeah - my thinking is the advantage of tree shaking that is very low at this point. If we bring the GL Context Wrapper back to its original state we're left with ~100 lines of code for just the WebPlatform. Shouldn't affect your bundle all that much.

That way I can circumvent having every client import a platform and feed that on the initial configuration constructor, while the majority of the cases will be on Web anyway.

If ever the WebPlatform layer gets too big or too much of a burden we can always make it tree shakeable

Choose a reason for hiding this comment

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

If ever the WebPlatform layer gets too big or too much of a burden we can always make it tree shakeable

Honest question: will anyone remember this discussion at that point?

I fully understand the "sensible defaults" argument and it's not my goal to make the whole API harder to use for everyone else. 😅
Wouldn't it be possible to have the injection somehow ensured at the settings object level, so that when we got to this point you'd have a guarantee it would be set? All we'd need then was an alternate path to configure Lightning that would require me to set everything up if I so wished, without having to include stuff I already knew I wouldn't need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well if we do it through the settings object you'd still have a default reference that will be included in the bundling process. Unless explicitly set externally from the renderer. So it would essentially move the problem up to the settings instead of RendererMain

and it doesn't really matter if anyone would remember this discussion, we can add a proper comment block explaining why/how/what and a future engineer can either draw the same conclusion or stumble upon the note. Hopefully the latter first but we've all been in the "oh here's actually a comment that explained what I just figured out and spent the last 2 hours on" situation 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

I second that it's probably going to be a useless complication to tree shake away the WebPlatform. In L2 the only use case for overriding the WebPlatform was done by creating a super class.

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.

4 participants