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

Flow 0.69 #6594

Merged
merged 3 commits into from
May 2, 2018
Merged

Flow 0.69 #6594

merged 3 commits into from
May 2, 2018

Conversation

jfirebaugh
Copy link
Contributor

Upgrade flow to 0.69. Not upgrading past that due to facebook/flow#6151.

One of the nice features in 0.68 is:

Previously, Flow would allow you to write if (foo.unknownProp) { ... }.
Now Flow disallows testing unknown properties in conditionals. If foo is a union type like { x: string } | { y: number }, x and y are known properties and z would be an unknown property

I remember finding a few bugs that would have been caught by this.

@jfirebaugh jfirebaugh requested a review from asheemmamoowala May 2, 2018 18:43
@@ -172,7 +172,7 @@ CompoundExpression.register(expressions, {
'feature-state': [
ValueType,
[StringType],
(ctx, [key]) => get(key.evaluate(ctx), ctx.state())
(ctx, [key]) => get(key.evaluate(ctx), ctx.featureState || {})
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a check for feature when using featureState? Feature state is only valid if there is a current feature. As it stands, it's possible to call Expression#evaluate with a null feature, but non-null featureState.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to fix it this way:

diff --git a/src/style-spec/expression/index.js b/src/style-spec/expression/index.js
index 1310db3f2..269f718ec 100644
--- a/src/style-spec/expression/index.js
+++ b/src/style-spec/expression/index.js
@@ -179,18 +179,18 @@ export class ZoomDependentExpression<Kind> {
 
 export type ConstantExpression = {
     kind: 'constant',
-    +evaluate: (globals: GlobalProperties, feature?: Feature) => any,
+    +evaluate: (globals: GlobalProperties) => any,
 }
 
 export type SourceExpression = {
     kind: 'source',
     isStateDependent: boolean,
-    +evaluate: (globals: GlobalProperties, feature?: Feature, featureState?: FeatureState) => any,
+    +evaluate: (globals: GlobalProperties, feature: Feature, featureState?: FeatureState) => any,
 };
 
 export type CameraExpression = {
     kind: 'camera',
-    +evaluate: (globals: GlobalProperties, feature?: Feature, featureState?: FeatureState) => any,
+    +evaluate: (globals: GlobalProperties, feature: Feature, featureState?: FeatureState) => any,
     +interpolationFactor: (input: number, lower: number, upper: number) => number,
     zoomStops: Array<number>
 };
@@ -198,7 +198,7 @@ export type CameraExpression = {
 export type CompositeExpression = {
     kind: 'composite',
     isStateDependent: boolean,
-    +evaluate: (globals: GlobalProperties, feature?: Feature, featureState?: FeatureState) => any,
+    +evaluate: (globals: GlobalProperties, feature: Feature, featureState?: FeatureState) => any,
     +interpolationFactor: (input: number, lower: number, upper: number) => number,
     zoomStops: Array<number>
 };

But this requires #6255 (comment), so I'd like to defer it.

However, in looking at this, I noticed that I'd failed to update ZoomDependentExpression#evaluate[WithoutErrorHandling] -- I've added additional render tests to cover that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems reasonable. #6255 is needed soon, and I don't see an actual code path at this time where this issue would come up.

@jfirebaugh jfirebaugh merged commit 4761e0b into master May 2, 2018
@jfirebaugh jfirebaugh deleted the flow-0.69 branch May 2, 2018 21:48
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.

2 participants