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.
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
Add CSS Map #1492
Add CSS Map #1492
Changes from 1 commit
664672b
73a7ffb
f5e8a25
0a22e90
184c044
8a028d5
dc772f8
c0fde8d
8002047
7c0ac9a
3f6a017
4748a80
3b6f848
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: moving
css-map.test.ts
topackages/babel-plugin/src/utils/__tests__/
perhaps(unless there's a reason to have it in the
css-prop
directory instead)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to be in
packages/babel-plugin/src/css-prop
, because they aren't unit tests against the functions ofcss-map.ts
. These tests rely on many things fromcss-prop
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah no worries 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something weird is happening here.
"success"==="danger"
can be statically evaluated tofalse
. Where does this comparison even come from though? I've got a feeling we're doing more work than needed with thecss()
call here, risks having spooky action.If I do this,
I shouldn't end up with each case considering all of
a,b,c,d and e
. I was explicit in eachThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, you are right.
But I think the selected variant is runtime variable at most of cases. Statically access to a variant is "by-product". Users probably should use
instead of
Also, bundler drops the dead code
('apple' === 'orange') && '.aaaabbbb'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quoting inconsistency here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: not sure what this extra pipe character does here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why the eslint plugin doesn't catch this 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
believe it or not. The extra pipe is enforced by prettier. 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it's outputting an intermediate form. It's preferable for us to consolidate things into a 1-pass compilation rather than having multiple passes, because this is lossy and prevents us from understanding the original context (which is useful for optimisations like dropping the
"success"==="danger"
case. What's inhibiting us from doing this in the same pass as thecss
-prop ->className
transform?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
um...good point. Let me look into it more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been an architectural thing.
foo ? 'color: red': 'color: green'
I think we can change this two-pass compilation into 1-pass by using local transform, as mentioned from another comment. I will make sure that I will spike this 1-pass compilation before PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having a bit of trouble understanding this nested ternary expression - could it be made more readable somehow?
e.g. use of parentheses
A ? (B ? C : D) : E
(A ? B : C) ? D : E
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or perhaps by extracting this ternary expression into a separate variable: