8000 start separating js.t from js_internal.t by jchavarri · Pull Request #798 · melange-re/melange · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

start separating js.t from js_internal.t #798

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.

Sign up for GitHub

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 8000 to your account

Jump to bottom
Merged
merged 5 commits into from
Oct 22, 2023
Merged

start separating js.t from js_internal.t #798

merged 5 commits into from
Oct 22, 2023

Conversation

jchavarri
Copy link
Member

Fixes #782.

jchavarri
jchavarri commented Oct 20, 2023
View reviewed changes
ppx/ast_literal.ml
@@ -31,17 +31,18 @@ type t = Longident.t

(* TODO should be renamed in to {!Js.fn} *)
(* TODO should be moved into {!Js.t} Later *)
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess part of this todo is being done here, not sure if it can be removed.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose we can remove it.

@jchavarri jchavarri mentioned this pull request Oct 20, 2023
Closed
jchavarri
jchavarri commented Oct 20, 2023
View reviewed changes
jscomp/runtime/js.mli Outdated
See also {!unsafe_lt}
*)

external unsafe_downgrade : 'a t -> 'a = "#unsafe_downgrade"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is prob the most controversial part of the PR, but before it was already accessible to consumers through Js__.Js_OO.unsafe_downgrade, so nothing has changed in terms of visibility.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, and we need it in js.ml because it needs Js.t. That's fine.

@jchavarri jchavarri mentioned this pull request Oct 20, 2023
Closed
anmonteiro
anmonteiro reviewed Oct 20, 2023
View reviewed changes
@anmonteiro anmonteiro force-pushed the split-jst-jsinternalt branch from ba83529 to 745b17c Compare October 21, 2023 00:54
anmonteiro
anmonteiro approved these changes Oct 21, 2023
View reviewed changes
Copy link
Member
@anmonteiro anmonteiro left a comment

Choose a reason for hiding this comment

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

This looks good if you like my changes.

Thanks for figuring this out! This an impactful user-facing change. I also think it deserves a changelog entry.

@anmonteiro anmonteiro force-pushed the split-jst-jsinternalt branch from 745b17c to 27153de Compare October 21, 2023 00:58
@jchavarri
Copy link
Member Author

continuing conversation from #798 (comment) because I deleted the .mli file.

I am not sure I understand why it's preferable to have the .mli file removed. I thought it would be beneficial to include an interface in a such a fundamental module like Js.

@anmonteiro
Copy link
Member

I am not sure I understand why it's preferable to have the .mli file removed. I thought it would be beneficial to include an interface in a such a fundamental module like Js.

In this case, because it wasn't adding much. It was just repeating the externals and adding module aliases. There were no signatures apart from Js_obj

jchavarri
jchavarri commented Oct 21, 2023
View reviewed changes
Copy link
Member Author
@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.

sorry, i had 3 inline comments pending but i forgot to submit the review :/

jscomp/runtime/js.ml
@@ -142,4 +171,14 @@ module WeakMap = Js_weakmap
module Cast = Js_cast
module MapperRt = Js_mapperRt

module Private = struct
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we could call this Unsafe. It is not really private. If we did that, then unsafe_downgrade could be just downgrade.

Copy link
Member

Choose a reason for hiding this comment

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

it is private, because it's not exposed as part of the public API. nothing ever uses Js_OO other than the compiler internals, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I must be missing something. I mean it is public in the sense that any user of the library can access it:

module T = Js.Private.Js_OO
let t = Js.Private.unsafe_downgrade

This is not what "private" means to me :) but I guess it's not that important.

Copy link
Member
@anmonteiro anmonteiro Oct 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

it's private as in it's internal code to the library that it's not part of the public API. I'm using a pattern that I've seen elsewhere, e.g. https://github.com/search?q=org%3Ajanestreet%20%22module%20private%22&type=code

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, so the name is a way to signal intent to the outside world ("use this at your own risk" etc).

jscomp/runtime/js.ml
@@ -142,4 +171,14 @@ module WeakMap = Js_weakmap
module Cast = Js_cast
module MapperRt = Js_mapperRt

module Private = struct
module Js_OO = struct
include Js_OO
Copy link
Member Author

Choose a reason for hiding this comment

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

Why is it necessary to bring whole Js_OO here? This might lead to unnecessary larger bundle right? Edit: never mind the bundle size remark, I got confused between include and atd inherit, which actually does duplicate code.

Copy link
Member

Choose a reason for hiding this comment

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

so Js_OO is not exposed at all. this allows it to be exposed in Js.Private.Js_OO.* and have the Ast_literal module be the same for every usage of Js_OO.

@anmonteiro
Copy link
Member

Whatever we do regarding the .mli needs to play well with the generated odoc documentation.

@jchavarri I'll leave that to you but feel free to request my help if you prefer

@jchavarri
Copy link
Member Author

Whatever we do regarding the .mli needs to play well with the generated odoc documentation.

I agree. I think it'll be better to tackle that when we focus on improving the odoc comments more broadly.

@jchavarri
Merge branch 'main' into split-jst-jsinternalt
d5f38ed 
* main:
  refactor: deduplicate functions in Utf8 logic (#803)
  test: remove cppo `#if false` for tests that we don't run (#804)
  doc(dev): Add CONTRIBUTING.md section about updating vendored submodules (#802)
@anmonteiro
< 628C div class="ml-n3 timeline-comment unminimized-comment comment previewable-edit js-task-list-container js-comment timeline-comment--caret" data-body-version="41acc5b1645a54a4130aa4868caba83219fa9523ad57df78b9b87c3787a66f9e">
Copy link
Member

Don't forget the changes entry.

@jchavarri
Copy link
Member Author

Couple of thoughts before we merge. Would anything be simplified if we moved the Js_OO module and the downgrade function to the ppx itself as a runtime library? I don't think it's used anywhere inside the melange.js library.

Also, will the change in this PR break anything in melange-compiler-libs? It seems to expect a specific path for Js_OO (without the Private namespacing):

https://github.com/melange-re/melange-compiler-libs/blob/eb8207c1b6744180095184194dfd843e06f9fad1/typing/oprint.ml#L309-L321

@anmonteiro
Copy link
Member

Would anything be simplified if we moved the Js_OO module and the downgrade function to the ppx itself as a runtime library? I don't think it's used anywhere inside the melange.js library.

well, this is the runtime library. I think it's in the right place.

Also, will the change in this PR break anything in melange-compiler-libs? It seems to expect a specific path for Js_OO (without the Private namespacing):

well... that's very well spotted, but that's already broken since the module is in fact Js__Js_OO, so we would need to fix it regardless.

@jchavarri
Copy link
Member Author

I created #806 to track the 2nd point.

@jchavarri jchavarri merged commit 69644e5 into main Oct 22, 2023
@jchavarri jchavarri deleted the split-jst-jsinternalt branch October 22, 2023 08:10
jchavarri added a commit that referenced this pull request Oct 22, 2023
@jchavarri @anmonteiro
start separating js.t from js_internal.t (#798)
5455684 
* start separating js.t from js_internal.t

* refactor: remove .mli, move some comments around

* refactor: how we expose Js_OO

* add changelog entry

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
jchavarri added a commit that referenced this pull request Oct 22, 2023
@jchavarri @anmonteiro
warnings: replace bs with mel (#799)
616bbdf 
* warnings: replace bs with mel

* fix playground tests

* update compiler libs

* chore: update flake.lock

* start separating js.t from js_internal.t (#798)

* start separating js.t from js_internal.t

* refactor: remove .mli, move some comments around

* refactor: how we expose Js_OO

* add changelog entry

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>

---------

Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
@jchavarri jchavarri mentioned this pull request Oct 22, 2023
Merged
@anmonteiro anmonteiro mentioned this pull request Oct 22, 2023
Merged
anmonteiro added a commit to anmonteiro/opam-repository that referenced this pull request Oct 22, 2023
@anmonteiro
[new release] melange (2 packages) (2.1.0)
ef73f68 
CHANGES:

- Add TRMC (Tail Recursion Modulo Cons) support
  ([melange-re/melange#743](melange-re/melange#743))
- [playground]: Add `melange.dom` to bundle
  ([melange-re/melange#779](melange-re/melange#779))
- Fix `Sys.argv` runtime to match declared type
  ([melange-re/melange#791](melange-re/melange#791))
- Make `'a Js.t` abstract (again), fixing a regression when bringing back
  OCaml-style objects BuckleScript
  ([melange-re/melange#786](melange-re/melange#786))
- Don't issue "unused attribute" warning for well-formed `@@@mel.config` in
  interface files ([melange-re/melange#800](melange-re/melange#800))
- Stop showing `Js__.Js_internal` in types and error messages
  ([melange-re/melange#798](melange-re/melange#798))
- Fix printing of OCaml-style objects and uncurried application
  ([melange-re/melange#807](melange-re/melange#807))
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
@anmonteiro @nberth
[new release] melange (2 packages) (2.1.0)
ac855d6 
CHANGES:

- Add TRMC (Tail Recursion Modulo Cons) support
  ([melange-re/melange#743](melange-re/melange#743))
- [playground]: Add `melange.dom` to bundle
  ([melange-re/melange#779](melange-re/melange#779))
- Fix `Sys.argv` runtime to match declared type
  ([melange-re/melange#791](melange-re/melange#791))
- Make `'a Js.t` abstract (again), fixing a regression when bringing back
  OCaml-style objects BuckleScript
  ([melange-re/melange#786](melange-re/melange#786))
- Don't issue "unused attribute" warning for well-formed `@@@mel.config` in
  interface files ([melange-re/melange#800](melange-re/melange#800))
- Stop showing `Js__.Js_internal` in types and error messages
  ([melange-re/melange#798](melange-re/melange#798))
- Fix printing of OCaml-style objects and uncurried application
  ([melange-re/melange#807](melange-re/melange#807))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anmonteiro anmonteiro anmonteiro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Js__.Js_internal leaks into type errors and LSP diagnostics
2 participants
@jchavarri @anmonteiro
0