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

melange-decoders: melange backend #64

Closed
wants to merge 9 commits into from
Closed

Conversation

actionshrimp
Copy link
Collaborator

@actionshrimp actionshrimp commented Aug 3, 2023

  • Combine bucklescript and melange implementations under js.
  • This is breaking for bucklescript users: They now have to e.g. open Decoders.Js_json instead of Decoders.Bs_json

When finished this should fix #59.

TODO:

  • update build to run the tests for the melange version
  • try consuming the lib from another project via opam pin to check the install hack works properly

bsconfig.json Outdated Show resolved Hide resolved
dune-project Show resolved Hide resolved
@mattjbray
Copy link
Owner

Is it possible to put all the js-related stuff under a melange/ subfolder? Something like:

  • Move src-js -> melange/src
  • Create melange/dune-project with (lang dune 3.8) and (using melange) (and revert /dune-project)
  • Dune will create melange/decoders-mel.opam
  • melange/src/dune does the (copy_files# ../../src/*.ml{,i})

@mattjbray
Copy link
Owner

Is it possible to put all the js-related stuff under a melange/ subfolder? Something like:

(And we'd need to figure out how to keep the bucklescript build working under this new layout too)

@actionshrimp actionshrimp force-pushed the dave/decoders-mel branch 2 times, most recently from d264bf6 to 62dc7f8 Compare August 18, 2023 09:08
Copy link
Owner

@mattjbray mattjbray left a comment

Choose a reason for hiding this comment

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

Nice.

js/src-js/shims_let_ops_.ml Outdated Show resolved Hide resolved
@actionshrimp
Copy link
Collaborator Author

Naming the melange package melange-decoders (as opposed to decoders-mel) to match the melange convention described here: melange-re/melange#688

@actionshrimp actionshrimp force-pushed the dave/decoders-mel branch 8 times, most recently from 9242007 to 8af2d45 Compare August 18, 2023 15:39
@actionshrimp actionshrimp force-pushed the dave/decoders-mel branch 2 times, most recently from 098adb8 to d196a30 Compare August 20, 2023 16:45
@actionshrimp actionshrimp force-pushed the dave/decoders-mel branch 2 times, most recently from b5264a3 to 766da4f Compare August 20, 2023 17:37
@actionshrimp actionshrimp changed the title Decoders-mel: melange backend melange-decoders: melange backend Aug 20, 2023
@actionshrimp
Copy link
Collaborator Author

Turns out we actually need (modes melange ..) for the decoders package itself in order to be able to use the Decoders sig in shared code - the shared code dune files will also need (modes melange ..) and (libraries decoders ..) for that to work.

So it looks like the melange dep and dune version bump is unavoidable. One temporary solution could be to leave this melange compatible version of decoders on a branch and rebase it off the main version. That way people who need melange support can pin the branch, and other people who don't use melange can use the regular version?

@actionshrimp actionshrimp mentioned this pull request Oct 4, 2023
@actionshrimp
Copy link
Collaborator Author

Superceded by #65

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.

Install bs-decoders from opam
2 participants