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

Initial support for $ReadOnlyArray #236

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

goodmind
Copy link
Collaborator

@goodmind goodmind commented Jul 24, 2019

Any-casting seems problematic with Object.freeze


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

return compareTypes(elementType, input.elementType);
}
else {
} else {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add subtyping here too

@jedwards1211
Copy link
Collaborator

jedwards1211 commented Jul 25, 2019

I skimmed the code...just realized, there's probably not a clean way to throw a runtime error if a $ReadOnlyArray is modified, is there? Seems like you're using Object.freeze to prevent the array from being modified, and I was thinking about using Proxies to throw errors but I assume there's not really a way to replace runtime references to the array with a Proxy object...

@jedwards1211
Copy link
Collaborator

jedwards1211 commented Jul 25, 2019

On second thought though, could statements like const foo: $ReadOnlyArray<X> = someArray be converted by babel-plugin-flow-runtime to const foo = wrapReadOnlyArray(someArray)?

@goodmind
Copy link
Collaborator Author

goodmind commented Jul 25, 2019

@jedwards1211 what about casting to any? It should basically unfreeze array which is impossible to do, that's why I'm having concern with it

@goodmind
Copy link
Collaborator Author

Ok, I see that any doesn't do anything to functions, so this is not a problem for $ReadOnlyArray.
There's also warning mode that only shows warning which is definitely no to Object.freeze

@goodmind
Copy link
Collaborator Author

Oh, it can trace assignments with Babel?

var foo: string;

foo = 1;

was converted to

import t from "flow-runtime";
var _fooType = t.string(),
    foo;

foo = _fooType.assert(1);

@jedwards1211
Copy link
Collaborator

Right you can look up where an identifier was declared using path.scope.getBinding(identifier.name)

@goodmind
Copy link
Collaborator Author

goodmind commented Jul 25, 2019

@jedwards1211 can I check what type annotation it has? This would work for $ReadOnlyArray or $ReadOnly, but wouldn't work for covariant properties
Also I think flow-runtime supports multiple files (imports), so not sure about this too

@jedwards1211
Copy link
Collaborator

jedwards1211 commented Jul 25, 2019

Yes, the binding identifier will have a type annotation node attached if the identifier is annotated, here is an example of what that looks like in the AST:
image

@goodmind goodmind marked this pull request as ready for review September 13, 2019 20:18
@jedwards1211
Copy link
Collaborator

@gajus what would you think about just using the same runtime type as Array for the time being (until one of us gets around to inserting a check before any statements that mutate the array, if that ever happens?)

@gajus
Copy link
Owner

gajus commented May 27, 2020

Sounds acceptable.

@jedwards1211
Copy link
Collaborator

jedwards1211 commented Dec 30, 2020

@goodmind I was thinking about this again because I use a lot of readonly types nowadays. It wouldn't actually make sense to insert any runtime checks for modifications on readonly-typed expressions, because such code paths would always throw (it's never in the developer's interest to write such a code path). For that reason it's really better to just rely on Flow to catch such cases, and not do anything special with the read-only status in babel-plugin-flow-runtime.

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.

3 participants