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

Remove use of RegisterClass #15671

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Remove use of RegisterClass #15671

wants to merge 2 commits into from

Conversation

deltakosh
Copy link
Contributor

@deltakosh deltakosh commented Oct 3, 2024

Ok folks! another new monster :)

I removed the need for side-effects for a LOT OF classes by replacing RegisterClass with the use of require.context

@ryantrem: please make sure that it is not a problem for the Viewer
@RaananW: I did not do the change for the FlowGraph because for some reasons that I do not know you are using FGxxx in the key for RegisterClass

The main class:
https://github.com/BabylonJS/Babylon.js/pull/15671/files#diff-6a2ff5432bcbb1632f27a94f714a7a50f7b3c4cac5d35bd7b7f8570746752e12

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 3, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@@ -1,6 +1,6 @@
import { NodeGeometryBlock } from "../nodeGeometryBlock";
import type { NodeGeometryConnectionPoint } from "../nodeGeometryBlockConnectionPoint";
import { RegisterClass } from "../../../Misc/typeStore";
import {} from "../../../Misc/typeStore";
Copy link
Member

Choose a reason for hiding this comment

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

should be removed I guess :-)

if (!_DiscoveredTypes) {
_DiscoveredTypes = {};
// Use require.context to import all modules
const requireModule = require.context(
Copy link
Member

Choose a reason for hiding this comment

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

Is this not webpack specific ? cause this would basically fail for anybody bundling without Webpack ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is THE question:) hence why I wanted @ryantrem to check

Copy link
Member

Choose a reason for hiding this comment

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

I can pull your branch and test it out with the viewer (which uses rollup) to be sure.

Copy link
Member

Choose a reason for hiding this comment

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

I commented about it. We can't use require.context if we want to stay esm friendly

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 3, 2024

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@deltakosh deltakosh marked this pull request as draft October 3, 2024 22:33
@RaananW
Copy link
Member

RaananW commented Oct 4, 2024

require.context is not ecma standard, and is webpack specific. adding this will make our package not work in browser anymore. Let's find a better way instead of that.

The idea of require.context is to be a synchronous import, but this is bundler specific (webpack in this case).

@RaananW
Copy link
Member

RaananW commented Oct 4, 2024

@RaananW: I did not do the change for the FlowGraph because for some reasons that I do not know you are using FGxxx in the key for RegisterClass

This will be changed as part of the changes I am making. there was a thought to shorten the names of the classes to save a few bytes, but this is not the right place to minimize input :-)

@deltakosh
Copy link
Contributor Author

Hey this is basically my question. Rollup and parcel have no equivalent so if we want to be side effects free what are our options regarding RegisterClass within the framework?

@ryantrem
Copy link
Member

ryantrem commented Oct 4, 2024

typeStore (e.g. RegisterClass, GetClass) is not considered a public API, it is internal only, correct? If the goal is to be able to tree shake out unused stuff, then it seems like we just need to replace this mechanism with well defined imports, right? I don't know the history though of typeStore and why we even have this functionality to register and retrieve types by names...

@deltakosh
Copy link
Contributor Author

It is internal to allow us to parse from serialized data (as the class will store its type so we can rebuild it at parse time). The question is then: how do you replace the feature?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants