Dynamic import in expression #6310
Conversation
|
Side note: I have to go now, I'll fix the formatting part couple of hours later 😓 |
|
Not to handle the case of // no need
await Belt.List |
| @@ -214,6 +221,19 @@ let expr_mapper ~async_context ~in_function_def (self : mapper) | |||
| the attribute to the whole expression, in general, when shuffuling the ast | |||
| it is very hard to place attributes correctly | |||
| *) | |||
| | Pexp_letmodule | |||
| (lid, ({pmod_desc = Pmod_ident {txt}; pmod_attributes} as me), expr) | |||
There was a problem hiding this comment.
Do you need this too, or are the use cases of this kind more rare?
| Pmod_constraint of module_expr * module_type
(* (ME : MT) *)There was a problem hiding this comment.
Oops, correct! I'll add it.
There was a problem hiding this comment.
I've added the case of Pmod_constraint ({pmod_desc = Pmod_ident _}, {pmty_desc = Pmty_ident mtyp_lid}) additionally, e.g. ME: MT. In other cases, for example, I don't think Pmty_alias or Pmty_with are applicable for dynamic import. Any thought?
jscomp/frontend/bs_builtin_ppx.ml
Outdated
| match has_local_module_name with | ||
| | Some _ -> None | ||
| | None -> | ||
| let open Ast_helper in |
There was a problem hiding this comment.
nit: remove the open, just makes the code more indirect
There was a problem hiding this comment.
Not sure I understand what the indirect means correctly:
1ed974f
jscomp/frontend/bs_builtin_ppx.ml
Outdated
| (* [ module __Belt_List__ = module type of Belt.List ] *) | ||
| let module_type_decls = | ||
| module_exprs | ||
| |> List.filter_map (fun ({pmod_desc} as me : Parsetree.module_expr) -> |
There was a problem hiding this comment.
nit: instead of filter-map twice and compose, why not compose the functions inside directly, and do only 1 filter-map.
| @@ -214,6 +221,19 @@ let expr_mapper ~async_context ~in_function_def (self : mapper) | |||
| the attribute to the whole expression, in general, when shuffuling the ast | |||
| it is very hard to place attributes correctly | |||
| *) | |||
| | Pexp_letmodule | |||
| (lid, ({pmod_desc = Pmod_ident {txt}; pmod_attributes} as me), expr) | |||
Good catch, thanks. There an issue in that case. I'll fix it. |
jscomp/frontend/bs_builtin_ppx.ml
Outdated
| ~module_type_lid:safe_module_type_lid me, | ||
| expr ); | ||
| } | ||
| self.expr self |
There was a problem hiding this comment.
This is a little dangerous.
One never applies the recursion to the whole expression, but to the subparts.
The danger is to create infinite loops.
jscomp/frontend/bs_builtin_ppx.ml
Outdated
| me, | ||
| expr ); | ||
| } | ||
| self.expr self |
|
Left comments for some final changes before merging. |
Thanks! f3ebaff |
|
Ready to merge? |
Fixes #6300
This PR fixes the issue #6300 that is formatting removes
awaitkeyword. This is not just a formatting issue, it's an issue because there is no transformation for when dynamic imports are used in expressions in addition to one of the structure_items,Pstr_module.This PR add the transformation for
Pstr_evalPstr_value.awaitin the expression