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

Consider having Document extending Graph #1594

Open
donmccurdy opened this issue Jan 3, 2025 · 0 comments
Open

Consider having Document extending Graph #1594

donmccurdy opened this issue Jan 3, 2025 · 0 comments
Labels
code health feature New enhancement or request package:core

Comments

@donmccurdy
Copy link
Owner

donmccurdy commented Jan 3, 2025

Currently the Document holds a reference to a Graph, from the property-graph package:

private _graph: Graph<Property> = new Graph<Property>();
private _root: Root = new Root(this._graph);
private _logger: ILogger = Logger.DEFAULT_INSTANCE;
/**
* Enables lookup of a Document from its Graph. For internal use, only.
* @internal
* @experimental
*/
private static _GRAPH_DOCUMENTS = new WeakMap<Graph<Property>, Document>();
/**
* Returns the Document associated with a given Graph, if any.
* @hidden
* @experimental
*/
public static fromGraph(graph: Graph<Property>): Document | null {
return Document._GRAPH_DOCUMENTS.get(graph) || null;
}

To keep function parameter simple though, we sometimes need to look up a Document from an arbitrary Property...

export function getTextureChannelMask(texture: Texture): number {
const document = Document.fromGraph(texture.getGraph())!;

... which works fine in general, but with Node.js continuing to delay the ecosystem from adopting ESM, it can be a source of hard-to-debug failures from dual package hazard. At some point I will need to drop CommonJS support entirely, but ...

It might be a nice simplification to merge the Document and Graph concepts by having Document extend Graph<Property>. Then there's one fewer concepts to think about, and one source of mystery bug in Node.js could go away. I think if I could do that and remove all use of instanceof, dual package hazard might not break end-user applications at all. May require a major release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health feature New enhancement or request package:core
Projects
None yet
Development

No branches or pull requests

1 participant