Skip to content
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

feat: produce a hard error for bs.* attributes #1034

Merged
merged 2 commits into from
Jan 25, 2024

Conversation

anmonteiro
Copy link
Member

  • this was brought up when migrating from v2 to v3.
  • the fact that melange stopped warning about bs.* attributes means that some modules now produce errors at runtime if they have old code -- this is unacceptable.
  • we now produce a hard error when finding a bs.* attribute

Copy link
Member

@jchavarri jchavarri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think this will be very useful to see what deps fail to build when publishing v3 in opam.

I noticed that extensions like mel.raw or mel.re aren't handled in the PR, do they go through a different path that doesn't have this problem?

ppx/ast_external_process.ml Show resolved Hide resolved
let warn_if_non_namespaced ~loc txt =
if not (Mel_ast_invariant.is_mel_attribute txt) then
Mel_ast_invariant.warn ~loc Deprecated_non_namespaced_attribute
let warn_if_bs_or_non_namespaced ~loc txt =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the function could be renamed to error_if_bs_or_non_namespaced.

@@ -130,7 +140,9 @@ let process_attributes_rev attrs : attr_kind * attribute list =
let process_pexp_fun_attributes_rev attrs =
List.fold_left
~f:(fun (st, acc) ({ attr_name = { txt; _ }; _ } as attr) ->
match txt with "mel.open" -> (true, acc) | _ -> (st, attr :: acc))
match txt with
| "mel.open" | "bs.open" -> (true, acc)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this attribute do? It's not documented in melange.re.

Also, should warn_if_bs_or_non_namespaced be called in this case too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mel.open extracts an OCaml exn from an external JS data source. Check some old docs here: https://melange.re/melange/Manual.html#__code_bs_open_code_type_safe_external_data_source_handling_since_1_7_0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anmonteiro anmonteiro merged commit fa1f67b into main Jan 25, 2024
4 checks passed
@anmonteiro anmonteiro deleted the anmonteiro/bs-attrs-hard-error branch January 25, 2024 07:03
anmonteiro added a commit that referenced this pull request Jan 29, 2024
* feat: produce a hard error for `bs.*` attributes

* chore: address code review items
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants