-
Notifications
You must be signed in to change notification settings - Fork 470
Add loc to each props in JSX4 #5937
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
Changes from 8 commits
f0507b7
588acf8
13d148a
81f3dfd
12a59aa
ced073d
b825937
41382f7
10421cd
b19e8c9
b381364
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -242,9 +242,9 @@ let recordFromProps ~loc ~removeKey callArguments = | |
(* let make = ({id, name, children}: props<'id, 'name, 'children>) *) | ||
let makePropsTypeParamsTvar namedTypeList = | ||
namedTypeList | ||
|> List.filter_map (fun (_isOptional, label, _, _interiorType) -> | ||
|> List.filter_map (fun (_isOptional, label, _, loc, _interiorType) -> | ||
if label = "key" then None | ||
else Some (Typ.var @@ safeTypeFromValue (Labelled label))) | ||
else Some (Typ.var ~loc @@ safeTypeFromValue (Labelled label))) | ||
|
||
let stripOption coreType = | ||
match coreType with | ||
|
@@ -268,7 +268,7 @@ let stripJsNullable coreType = | |
let makePropsTypeParams ?(stripExplicitOption = false) | ||
?(stripExplicitJsNullableOfRef = false) namedTypeList = | ||
namedTypeList | ||
|> List.filter_map (fun (isOptional, label, _, interiorType) -> | ||
|> List.filter_map (fun (isOptional, label, _, loc, interiorType) -> | ||
if label = "key" then None | ||
(* TODO: Worth thinking how about "ref_" or "_ref" usages *) | ||
else if label = "ref" then | ||
|
@@ -277,7 +277,7 @@ let makePropsTypeParams ?(stripExplicitOption = false) | |
For example, if JSX ppx is used for React Native, type would be different. | ||
*) | ||
match interiorType with | ||
| {ptyp_desc = Ptyp_var "ref"} -> Some (refType Location.none) | ||
| {ptyp_desc = Ptyp_var "ref"} -> Some (refType loc) | ||
| _ -> | ||
(* Strip explicit Js.Nullable.t in case of forwardRef *) | ||
if stripExplicitJsNullableOfRef then stripJsNullable interiorType | ||
|
@@ -287,21 +287,39 @@ let makePropsTypeParams ?(stripExplicitOption = false) | |
else if isOptional && stripExplicitOption then stripOption interiorType | ||
else Some interiorType) | ||
|
||
let makeLabelDecls ~loc namedTypeList = | ||
namedTypeList | ||
|> List.map (fun (isOptional, label, attrs, interiorType) -> | ||
if label = "key" then | ||
Type.field ~loc ~attrs:(optionalAttrs @ attrs) {txt = label; loc} | ||
interiorType | ||
else if isOptional then | ||
Type.field ~loc ~attrs:(optionalAttrs @ attrs) {txt = label; loc} | ||
(Typ.var @@ safeTypeFromValue @@ Labelled label) | ||
else | ||
Type.field ~loc ~attrs {txt = label; loc} | ||
(Typ.var @@ safeTypeFromValue @@ Labelled label)) | ||
let makeLabelDecls namedTypeList = | ||
let rec mem_fst x = function | ||
| [] -> false | ||
| (a, _) :: l -> compare a (fst x) = 0 || mem_fst x l | ||
in | ||
let rec duplicated = function | ||
| [] -> None | ||
| hd :: tl -> if mem_fst hd tl then Some hd else duplicated tl | ||
in | ||
let duplicatedProp = | ||
(* Find the duplicated prop from the back *) | ||
namedTypeList |> List.rev | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now we always have to do an additional Could this be done in a more efficient way, avoiding additional List allocations in the standard case (no duplicate props)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elavorate your thought? Do you mean that finding the duplicated prop without There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, something like that, it would be ideal if we could perform the check without creating any additional lists. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agree! I'll work on it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for being late, I'm stuck with tasks from company 🥲 How does it look? 10421cd Looking a bit verbose, but I think it is avoiding the list relocation before checking for duplicated props. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks good to me! Thanks a lot! |
||
|> List.map (fun (_, label, _, loc, _) -> (label, loc)) | ||
|> duplicated | ||
in | ||
match duplicatedProp with | ||
| Some (label, loc) -> | ||
React_jsx_common.raiseError ~loc "JSX: found the duplicated prop `%s`" label | ||
| None -> | ||
namedTypeList | ||
|> List.map (fun (isOptional, label, attrs, loc, interiorType) -> | ||
if label = "key" then | ||
Type.field ~loc ~attrs:(optionalAttrs @ attrs) {txt = label; loc} | ||
interiorType | ||
else if isOptional then | ||
Type.field ~loc ~attrs:(optionalAttrs @ attrs) {txt = label; loc} | ||
(Typ.var @@ safeTypeFromValue @@ Labelled label) | ||
else | ||
Type.field ~loc ~attrs {txt = label; loc} | ||
(Typ.var @@ safeTypeFromValue @@ Labelled label)) | ||
|
||
let makeTypeDecls propsName loc namedTypeList = | ||
let labelDeclList = makeLabelDecls ~loc namedTypeList in | ||
let labelDeclList = makeLabelDecls namedTypeList in | ||
(* 'id, 'className, ... *) | ||
let params = | ||
makePropsTypeParamsTvar namedTypeList | ||
|
@@ -702,17 +720,22 @@ let argToType ~newtypes ~(typeConstraints : core_type option) types | |
in | ||
match (type_, name, default) with | ||
| Some type_, name, _ when isOptional name -> | ||
(true, getLabel name, attrs, {type_ with ptyp_attributes = optionalAttrs}) | ||
( true, | ||
getLabel name, | ||
attrs, | ||
loc, | ||
{type_ with ptyp_attributes = optionalAttrs} ) | ||
:: types | ||
| Some type_, name, _ -> (false, getLabel name, attrs, type_) :: types | ||
| Some type_, name, _ -> (false, getLabel name, attrs, loc, type_) :: types | ||
| None, name, _ when isOptional name -> | ||
( true, | ||
getLabel name, | ||
attrs, | ||
loc, | ||
Typ.var ~loc ~attrs:optionalAttrs (safeTypeFromValue name) ) | ||
:: types | ||
| None, name, _ when isLabelled name -> | ||
(false, getLabel name, attrs, Typ.var ~loc (safeTypeFromValue name)) | ||
(false, getLabel name, attrs, loc, Typ.var ~loc (safeTypeFromValue name)) | ||
:: types | ||
| _ -> types | ||
|
||
|
@@ -721,10 +744,12 @@ let argWithDefaultValue (name, default, _, _, _, _) = | |
| Some default when isOptional name -> Some (getLabel name, default) | ||
| _ -> None | ||
|
||
let argToConcreteType types (name, attrs, _loc, type_) = | ||
let argToConcreteType types (name, attrs, loc, type_) = | ||
match name with | ||
| name when isLabelled name -> (false, getLabel name, attrs, type_) :: types | ||
| name when isOptional name -> (true, getLabel name, attrs, type_) :: types | ||
| name when isLabelled name -> | ||
(false, getLabel name, attrs, loc, type_) :: types | ||
| name when isOptional name -> | ||
(true, getLabel name, attrs, loc, type_) :: types | ||
| _ -> types | ||
|
||
let check_string_int_attribute_iter = | ||
|
@@ -1306,7 +1331,8 @@ let transformSignatureItem ~config _mapper item = | |
makePropsRecordTypeSig ~coreTypeOfAttr ~typVarsOfCoreType "props" | ||
psig_loc | ||
((* If there is Nolabel arg, regard the type as ref in forwardRef *) | ||
(if !hasForwardRef then [(true, "ref", [], refType Location.none)] | ||
(if !hasForwardRef then | ||
[(true, "ref", [], Location.none, refType Location.none)] | ||
else []) | ||
@ namedTypeList) | ||
in | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,7 @@ | ||
@@jsxConfig({version: 4, mode: "automatic"}) | ||
|
||
module C0 = { | ||
type props<'priority, 'text> = { | ||
priority: 'priority, | ||
text?: 'text, | ||
} | ||
type props<'priority, 'text> = {priority: 'priority, text?: 'text} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wonder why this PR change leads to remove the trailing comma. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably formatted differently now because of the loc changes. |
||
|
||
@react.component | ||
let make = ({priority: _, ?text, _}: props<'priority, 'text>) => { | ||
|
@@ -23,10 +20,7 @@ module C0 = { | |
} | ||
|
||
module C1 = { | ||
type props<'priority, 'text> = { | ||
priority: 'priority, | ||
text?: 'text, | ||
} | ||
type props<'priority, 'text> = {priority: 'priority, text?: 'text} | ||
|
||
@react.component | ||
let make = ({priority: p, ?text, _}: props<'priority, 'text>) => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,2 @@ | ||
type props<'x, 'ref> = { | ||
x: 'x, | ||
ref?: 'ref, | ||
} | ||
type props<'x, 'ref> = {x: 'x, ref?: 'ref} | ||
let make: React.componentLike<props<string, ReactDOM.Ref.currentDomRef>, React.element> |
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 change is not needed anymore right?
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.
Correct, I will revert it.