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

[compiler] Custom type definitions in config #30670

Closed
wants to merge 2 commits into from

Conversation

mvitousek
Copy link
Contributor

@mvitousek mvitousek commented Aug 13, 2024

Stack from ghstack (oldest at bottom):

Summary:
This PR allows custom type definitions for globals to be loaded in the Forget config. We use a slightly different representation than we do internally, in order to ensure that things are well formed before we pass them to the shape registry. The most fundamental issue is that we introduce a notion of IDs (expected to be unique) for types: we can introduce a type and then refer to that type later by a unique ID in a function return type if we have, for example, a global value that returns itself when called.

Summary:
This PR allows custom type definitions for globals to be loaded in the Forget config. We use a slightly different representation than we do internally, in order to ensure that things are well formed before we pass them to the shape registry. The most fundamental issue is that we introduce a notion of IDs (expected to be unique) for types: we can introduce a type and then refer to that type later by a unique ID in a function return type if we have, for example, a global value that returns itself when called.

[ghstack-poisoned]
Copy link

vercel bot commented Aug 13, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 16, 2024 7:10pm

mvitousek added a commit that referenced this pull request Aug 13, 2024
Summary:
This PR allows custom type definitions for globals to be loaded in the Forget config. We use a slightly different representation than we do internally, in order to ensure that things are well formed before we pass them to the shape registry. The most fundamental issue is that we introduce a notion of IDs (expected to be unique) for types: we can introduce a type and then refer to that type later by a unique ID in a function return type if we have, for example, a global value that returns itself when called.

ghstack-source-id: 46791e48af2410996974747cc4849acfa63fdc20
Pull Request resolved: #30670
@@ -756,6 +847,7 @@ export class Environment {
return (
moduleName.toLowerCase() === 'react' ||
moduleName.toLowerCase() === 'react-dom' ||
[...this.#globals.keys()].includes(moduleName) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this?

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 predicate gates giving types to imports from globals here. Gonna rename this function to isKnownTypedModule!

Comment on lines 546 to 558
if (type.fn.returnType == null) {
func.returnType = {kind: 'Poly'};
} else if (type.fn.returnType === 'array') {
func.returnType = {kind: 'Object', shapeId: BuiltInArrayId};
} else if (refMap.has(type.fn.returnType)) {
func.returnType = refMap.get(type.fn.returnType)!;
} else {
const cur = toSet.get(type.fn.returnType);
toSet.set(type.fn.returnType, (ty: BuiltInType) => {
cur?.(ty);
func.returnType = ty;
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

does this allow custom types to refer to builtin types? would be nice if that was the case (helper function that returns a ref, for example)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah! Just need to add 'ref', etc to TypeReferenceSchema and then link it with the builtin ref shape.


custom(x);
custom.prop(x);
custom.notPresent(y);
Copy link
Contributor

Choose a reason for hiding this comment

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

how is this case handled? we don't have a declaration for it so i would expect it to assume it's a mutation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's correct--that's why x is memoized (its mutable range doesn't extend beyond the hook call) but y is not memoized (custom.notPresent() may mutate it, so its range extends over the hook call)

Summary:
This PR allows custom type definitions for globals to be loaded in the Forget config. We use a slightly different representation than we do internally, in order to ensure that things are well formed before we pass them to the shape registry. The most fundamental issue is that we introduce a notion of IDs (expected to be unique) for types: we can introduce a type and then refer to that type later by a unique ID in a function return type if we have, for example, a global value that returns itself when called.

[ghstack-poisoned]
mvitousek added a commit that referenced this pull request Aug 16, 2024
Summary:
This PR allows custom type definitions for globals to be loaded in the Forget config. We use a slightly different representation than we do internally, in order to ensure that things are well formed before we pass them to the shape registry. The most fundamental issue is that we introduce a notion of IDs (expected to be unique) for types: we can introduce a type and then refer to that type later by a unique ID in a function return type if we have, for example, a global value that returns itself when called.

ghstack-source-id: 48a765366e8af9f3b74454978161deb37db1b455
Pull Request resolved: #30670
Copy link
Contributor

@josephsavona josephsavona left a comment

Choose a reason for hiding this comment

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

nice! thanks for confirming how that works, and i guess we can always add more cases for the built-in types as we need them

Copy link
Contributor

@josephsavona josephsavona left a comment

Choose a reason for hiding this comment

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

As part of internal rollout I needed more flexibility than this (ability to supply a type for arbitrary modules of a certain pattern) so I landed #30771. Let's chat when you're back, i'm happy to iterate on the design (i borrowed ideas from this PR)

Copy link

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Nov 22, 2024
Copy link

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!

@github-actions github-actions bot closed this Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Resolution: Stale Automatically closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants