Preparatory expression refactors #5407
Conversation
|
@jfirebaugh couple of preparatory changes that I'd like to get merged to reduce need for invasive rebasing in #5253 and #5193 |
src/style-spec/expression/index.js
Outdated
| expression = createExpression(expression, expectedType); | ||
| function createExpressionWithErrorHandling(expression: mixed, options: StyleExpressionOptions): StyleExpression { | ||
| expression = createExpression(expression, options); | ||
| const defaultValue = options.defaultValue === undefined ? null : options.defaultValue; |
There was a problem hiding this comment.
Should we have errorHandling: boolean as an option and then expose only createExpression()?
There was a problem hiding this comment.
Yeah, we need to unify the two somehow for #5222 and #5370. I was trying to figure out a clean way to do it. Could we use an error return type by default, and push the try/catch up to StyleDeclaration#calculate? I avoided doing this in previous performance PRs because it would make the expression benchmarks not strictly comparable, but I'm 👍 on doing it in an independent PR.
Also, we should use Color ubiquitously so unwrap becomes unnecessary.
There was a problem hiding this comment.
Could we use an error return type by default, and push the try/catch up to
StyleDeclaration#calculate?
Hmm, nope, that regresses layout performance, presumably because constant "evaluation" is also wrapped in try/catch. https://bl.ocks.org/anonymous/raw/2b247bab7e59c3f83fa75ad8c7f2921d/
There was a problem hiding this comment.
@jfirebaugh could we just decide whether to do error handling based on the presence or absence of defaultValue?
src/style-spec/expression/index.js
Outdated
| @@ -24,19 +24,31 @@ export type Feature = { | |||
| +properties: {[string]: any} | |||
| }; | |||
|
|
|||
| export type StyleExpressionContext = 'declaration' | 'filter'; | |||
There was a problem hiding this comment.
'declaration' → 'property'?
src/style-spec/expression/index.js
Outdated
| expression = createExpression(expression, expectedType); | ||
| function createExpressionWithErrorHandling(expression: mixed, options: StyleExpressionOptions): StyleExpression { | ||
| expression = createExpression(expression, options); | ||
| const defaultValue = options.defaultValue === undefined ? null : options.defaultValue; |
There was a problem hiding this comment.
Yeah, we need to unify the two somehow for #5222 and #5370. I was trying to figure out a clean way to do it. Could we use an error return type by default, and push the try/catch up to StyleDeclaration#calculate? I avoided doing this in previous performance PRs because it would make the expression benchmarks not strictly comparable, but I'm 👍 on doing it in an independent PR.
Also, we should use Color ubiquitously so unwrap becomes unnecessary.
anandthakker
force-pushed
the
expressions-other-contexts
branch
from
b571208
to
b79f9fc
Compare
anandthakker
force-pushed
the
expressions-other-contexts
branch
from
b79f9fc
to
93d6857
Compare
anandthakker
force-pushed
the
expressions-other-contexts
branch
from
93d6857
to
7923f77
Compare
No description provided.