feat(ecmascript): support dynamic tenary operator jsvalue #6452
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
9 Ignored Deployments
|
|
✅ This change can build |
🟢 CI successful 🟢Thanks |
Linux Benchmark for e99af59Click to view benchmark
|
|
jridgewell
approved these changes
Nov 15, 2023
| if test.is_truthy() == Some(true) { | ||
| *value = take(cons); | ||
| true | ||
| } else if test.is_falsy() == Some(true) { |
There was a problem hiding this comment.
This is just test.is_truty() == Some(false), right? We can this into a branch above and save some cycles.
There was a problem hiding this comment.
I think so, this is borrowing existing references from elsewhere (I recall it's binary operation or something)
There was a problem hiding this comment.
Yeah this can be a match on is_truty
sokra
reviewed
Nov 15, 2023
| @@ -1224,6 +1246,12 @@ impl JsValue { | |||
| op.joiner(), | |||
| b.explain_internal_inner(hints, indent_depth, depth, unknown_depth), | |||
| ), | |||
| JsValue::Tenary(_, test, cons, alt) => format!( | |||
| "({}?{}:{})", | |||
There was a problem hiding this comment.
A bit more spacing here would be nice
sokra
reviewed
Nov 15, 2023
| @@ -1,5 +1,7 @@ | |||
| const a = import(true ? "a" : "b") | |||
There was a problem hiding this comment.
This is testing your tenary support and should evaluate to a
There was a problem hiding this comment.
I wonder how this already works before
There was a problem hiding this comment.
Those tenary passes evaluation at
3c2d29c#diff-2c7634641094d97670b756458b73ab7bc5f6fba2da97471427a6c95795b8562dR429, while dynamic tenary evaluation doesn't seem to pass, so it goes to jsvalue:alternatives.
sokra
reviewed
Nov 15, 2023
| @@ -3,10 +3,23 @@ | |||
| 0 -> 2 call = import*0*("a") | |||
kwonoj
force-pushed
the
jsvalue-tenary
branch
from
9d1955d
to
408e057
Compare
Linux Benchmark for ce0ac01Click to view benchmark
|
kwonoj
force-pushed
the
jsvalue-tenary
branch
from
408e057
to
b7a889f
Compare
Linux Benchmark for 4ef8aa7Click to view benchmark
|
kwonoj
force-pushed
the
jsvalue-tenary
branch
from
b7a889f
to
ec280a4
Compare
Linux Benchmark for 94f2585Click to view benchmark
|
kwonoj
force-pushed
the
jsvalue-tenary
branch
from
ec280a4
to
c78bd41
Compare
Linux Benchmark for fb3cd66Click to view benchmark
|
sokra
added a commit
that referenced
this pull request
Nov 17, 2023
wbinnssmith
pushed a commit
that referenced
this pull request
Nov 17, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
context: https://vercel.slack.com/archives/C03EWR7LGEN/p1699990767027949
Currently, if the input source have a non-statically analyzable tenary operator
it becomes to the output like
that takes first value of tenary regardless of the condition. In the analyze phase if tenary is dynamic it retuns
JsValue::Alternatives, and it takes the first available value immediately.PR changes its behavior by having new type of JsValue for tenary - if condExpr is not statically analyzable. I tried to use existing JsValue but that doesn't seem to work easily out of the box. However I may miss some obvious so PR might be refactored more simple way instead.
Closes PACK-1963