feat(compiler): support target envs #1160

cdaringe · 2021-10-19T21:03:09Z

Problem

See #1157

Solution

Add config & cli options for target runtime
Add conditional //If: ... handling to include target runtime artifacts in compilation

runtime/fs_fake.js

hhugo · 2021-10-19T23:21:40Z

I suspect we need to do something in fs.js whenever we try to use MlNodeDevice.
Implement some kind of optional //Requires: maybe ?

hhugo · 2021-10-19T23:25:04Z

We need some test in compiler/tests-compiler

compiler/lib/linker.ml

cdaringe · 2021-10-21T06:29:41Z

Ok, so here's what I've tried:

Support //If: <target_env> annotations. by default, when no annotations are present, it assumes the status quo. The status quo is implicitly an isomorphic case--support javascript running in node or browser.
If there is a target_env specific implementation available specified by an //If: <target_env> annotation, allow it to override the isomorphic case. In fs_node.js, for example, i provided empty impls for the browser. This should be fine and good, as these functions should never be invoked, as someone targeting the browser should never be calling a FS API. Someone will undoubtedly try, and the will get the all-too-familiar cannot call FIELD_NAME of undefined error :). This is an acceptable/desirable state.
Update the fragment data model to included the annotation list, so we can conditionally swap the target_env implementations

runtime/fs_node.js

cdaringe · 2021-11-02T07:34:18Z

Implementation complete! Currently, I only reduce the fs_node.js module. This brings an --opt=3 let () = print_endline ... prog from 19k => 15k :) I see some possible other wins as well, which we can riff on in a different issue or changeset. I tested the examples/ in browser as well.

Ready for review

cdaringe · 2021-11-05T03:29:23Z

Looks like yesterday's build was only grumpy because I forgot a CHANGES.md patch. Patch sent, ready for next CI attempt

cdaringe · 2021-11-11T08:47:52Z

Hey @hhugo, truly no rush, but ready for another round of reviews. I believe the feature is complete :)

compiler/bin-js_of_ocaml/cmd_arg.ml

compiler/bin-js_of_ocaml/compile.ml

compiler/lib/linker.ml

cdaringe

thanks for the helpful feedback. comments all replied to, most with patches :)

compiler/bin-js_of_ocaml/compile.ml

compiler/lib/linker.ml

cdaringe · 2021-11-12T21:01:07Z

Thanks for all of the patience with this back and forth conversation. I acted on all of your feedback 👍

CHANGES.md

compiler/lib/linker.ml

cdaringe · 2021-11-21T01:08:36Z

patches sent for all feedback :)

hhugo · 2021-11-29T11:17:05Z

README.md

@@ -55,7 +55,7 @@ ocamlfind ocamlc -package js_of_ocaml -package js_of_ocaml-ppx -linkpkg -o cubes
 Then, run the `js_of_ocaml` compiler to produce JavaScript code:

 ```
-js_of_ocaml cubes.byte
+js_of_ocaml --target-env=browser cubes.byte


Maybe add a section to the README to explain the target-env flag. We should update the manual as well

compiler/lib-runtime/gen/gen.ml

hhugo · 2021-11-29T11:44:02Z

I've pushed some changes to your branch directly, hope you don't mind.

Things I've done:

refactor load_fragment to try to make the logic easier to follow
remove some let open as it make code harder to understand

Let me know what you think

hhugo · 2021-11-29T13:25:45Z

I think I'm happy with where we are, modulo some missing documentation.
I'm planning to squash all commits into a single one at the end, unless you disagree.
Let me know what you think

hhugo · 2021-11-30T10:57:41Z

@cdaringe, will you be able to write some documentation ?

cdaringe · 2021-11-30T20:45:54Z

Ya, I can indeed.

On Tue, Nov 30, 2021 at 2:57 AM hhugo ***@***.***> wrote: @cdaringe <https://github.com/cdaringe>, will you be able to write some documentation ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1160 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHU57PVNZVS7RDO63WO5DDUOSU3BANCNFSM5GKCROGQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- Sent from Gmail Mobile

cdaringe · 2021-12-05T01:52:07Z

hey @hhugo, sorry for the delay. I've been ill for a week.

956e1ae

it's not much, but felt it was probably all that was warranted, based on the current docs organization
i saw make doc, but not clear on how to actually render .wiki files

let me know if you want a deeper writeup somewhere

hhugo · 2021-12-07T15:31:57Z

Thanks for the hard work.

smorimoto · 2021-12-07T15:48:01Z

HELL YEAH! Thank you for your efforts on this @cdaringe 🙂

…s_of_ocaml-ppx_deriving_json, js_of_ocaml-ppx, js_of_ocaml-lwt and js_of_ocaml-compiler (4.0.0) CHANGES: ## Features/Changes * Compiler: add --target-env flag, for JS runtime specific compilation targets (ocsigen/js_of_ocaml#1160). * Compiler: static evaluation of backend_type (ocsigen/js_of_ocaml#1166) * Compiler: speedup emitting js files (ocsigen/js_of_ocaml#1174) * Compiler: simplify (a | 0) >>> 0 into (a >>> 0) (ocsigen/js_of_ocaml#1177) * Compiler: improve static evaluation of cond (ocsigen/js_of_ocaml#1178) * Compiler: be more consistent dealing with js vs ocaml strings (ocsigen/js_of_ocaml#984) * Compiler: Compiler: add BigInt to provided symbols (fix ocsigen/js_of_ocaml#1168) (ocsigen/js_of_ocaml#1191) * Compiler: use globalThis, drop joo_global_object ocsigen/js_of_ocaml#1193 * Compiler: new -Werror flag to turn wanrings into errors (ocsigen/js_of_ocaml#1222) * Compiler: make the inlining less agressive, reduce size, improve pref (ocsigen/js_of_ocaml#1220) * Compiler: rename internal library js_of_ocaml-compiler.runtime to js_of_ocaml-compiler.runtime-files * Lib: new runtime library to improve compatibility with Brr and gen_js_api * Lib: add messageEvent to Dom_html (ocsigen/js_of_ocaml#1164) * Lib: add PerformanceObserver API (ocsigen/js_of_ocaml#1164) * Lib: add CSSStyleDeclaration.{setProperty, getPropertyValue, getPropertyPriority, removeProperty} (ocsigen/js_of_ocaml#1170) * Lib: make window.{inner,outer}{Width,Height} non-optional * Lib: introduce Js_of_ocaml.Js_error module, deprecate Js_of_ocaml.Js.Error exception. * Lib: add deprecation warning for deprecated code * PPX: json can now be derived for mutable records (ocsigen/js_of_ocaml#1184) * Runtime: use crypto.getRandomValues when available (ocsigen/js_of_ocaml#1209) * Misc: move js_of_ocaml-ocamlbuild out to its own repo * Misc: add support for OCaml 4.14 (ocsigen/js_of_ocaml#1173) ## Bug fixes * Compiler: fix sourcemap warning for empty cma (ocsigen/js_of_ocaml#1169) * Compiler: Strengthen bound checks. (ocsigen/js_of_ocaml#1172) * Compiler: fix `--wrap-with-fun` under node (ocsigen/js_of_ocaml#653, ocsigen/js_of_ocaml#1171) * Compiler: fix parsing of annotaions in js stubs (ocsigen/js_of_ocaml#1212, fix ocsigen/js_of_ocaml#1213) * Ppx: allow apostrophe in lident (fix ocsigen/js_of_ocaml#1183) (ocsigen/js_of_ocaml#1192) * Runtime: fix float parsing in hexadecimal form * Runtime: fix implementation of caml_js_instanceof * Graphics: fix mouse_{x,y} (ocsigen/js_of_ocaml#1206)

TheSpyder · 2022-04-26T02:04:20Z

how do I set this option in a dune build? I tried (js_of_ocaml (flags (:standard --target-env=browser))) but then dune passes the flag to js_of_ocaml link where it's an unknown option.

TheSpyder · 2022-04-26T02:30:19Z

ah that's ocaml/dune#5563, my apologies

cdaringe mentioned this pull request Oct 19, 2021

how to optimize a common.js artifact for the target env #1157

Closed

cdaringe commented Oct 19, 2021

View reviewed changes

runtime/fs_fake.js Outdated Show resolved Hide resolved

hhugo reviewed Oct 19, 2021

View reviewed changes

runtime/fs_fake.js Outdated Show resolved Hide resolved

hhugo reviewed Oct 19, 2021

View reviewed changes

runtime/fs_fake.js Outdated Show resolved Hide resolved

hhugo reviewed Oct 19, 2021

View reviewed changes

compiler/lib/linker.ml Outdated Show resolved Hide resolved

cdaringe commented Oct 20, 2021

View reviewed changes

compiler/lib/linker.ml Outdated Show resolved Hide resolved

cdaringe commented Oct 21, 2021

View reviewed changes

runtime/fs_node.js Show resolved Hide resolved

cdaringe force-pushed the feat/support-env-targets branch 2 times, most recently from 7cb10af to 98d1d5a Compare November 1, 2021 01:55

cdaringe marked this pull request as ready for review November 2, 2021 07:04

cdaringe requested a review from hhugo November 2, 2021 07:45

hhugo reviewed Nov 11, 2021

View reviewed changes

compiler/bin-js_of_ocaml/cmd_arg.ml Outdated Show resolved Hide resolved

compiler/bin-js_of_ocaml/compile.ml Outdated Show resolved Hide resolved

compiler/bin-js_of_ocaml/compile.ml Outdated Show resolved Hide resolved

compiler/lib/linker.ml Outdated Show resolved Hide resolved

hhugo reviewed Nov 11, 2021

View reviewed changes

compiler/lib/linker.ml Outdated Show resolved Hide resolved

cdaringe commented Nov 12, 2021

View reviewed changes

compiler/bin-js_of_ocaml/compile.ml Outdated Show resolved Hide resolved

compiler/lib/linker.ml Outdated Show resolved Hide resolved

cdaringe requested a review from hhugo November 12, 2021 07:17

hhugo reviewed Nov 12, 2021

View reviewed changes

compiler/lib/linker.ml Outdated Show resolved Hide resolved

compiler/lib/linker.ml Outdated Show resolved Hide resolved

hhugo reviewed Nov 18, 2021

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

hhugo reviewed Nov 18, 2021

View reviewed changes

compiler/lib/linker.ml Outdated Show resolved Hide resolved

hhugo reviewed Nov 18, 2021

View reviewed changes

compiler/lib/linker.ml Outdated Show resolved Hide resolved

hhugo reviewed Nov 18, 2021

View reviewed changes

compiler/lib/linker.ml Outdated Show resolved Hide resolved

cdaringe force-pushed the feat/support-env-targets branch from b33660d to 97f1e42 Compare November 21, 2021 00:29

cdaringe requested a review from hhugo November 21, 2021 01:07

cdaringe requested a review from hhugo November 28, 2021 05:56

hhugo added 2 commits November 29, 2021 12:02

some refactoring

f125bd8

update dune

3f97b57

hhugo reviewed Nov 29, 2021

View reviewed changes

hhugo added 2 commits November 29, 2021 12:29

update doc

3ac3c01

less local open

3b812a4

hhugo reviewed Nov 29, 2021

View reviewed changes

compiler/lib-runtime/gen/gen.ml Outdated Show resolved Hide resolved

hhugo added 4 commits November 29, 2021 12:56

update tests

1ec268c

fix runtime, update tests

c4dc96e

more cleanup

6351ddc

more cleanup

f36b5ac

hhugo added 4 commits November 29, 2021 14:36

fix runtime check

e28a64e

remove duplicated code

62aee9d

remove duplicated code

a85ef40

typo

2419f43

docs: record --target-env

956e1ae

cleanup

9740494

hhugo merged commit 54e6659 into ocsigen:master Dec 7, 2021

cdaringe deleted the feat/support-env-targets branch December 7, 2021 16:25

hhugo mentioned this pull request Jan 24, 2022

[new release] js_of_ocaml, js_of_ocaml-tyxml, js_of_ocaml-toplevel, js_of_ocaml-ppx_deriving_json, js_of_ocaml-ppx, js_of_ocaml-lwt and js_of_ocaml-compiler (4.0.0) ocaml/opam-repository#20533

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(compiler): support target envs #1160

feat(compiler): support target envs #1160

cdaringe commented Oct 19, 2021 •

edited

hhugo commented Oct 19, 2021

hhugo commented Oct 19, 2021

cdaringe commented Oct 21, 2021 •

edited

cdaringe commented Nov 2, 2021 •

edited

cdaringe commented Nov 5, 2021 •

edited

cdaringe commented Nov 11, 2021

cdaringe left a comment

cdaringe commented Nov 12, 2021

cdaringe commented Nov 21, 2021

hhugo Nov 29, 2021 •

edited

hhugo commented Nov 29, 2021

hhugo commented Nov 29, 2021

hhugo commented Nov 30, 2021

cdaringe commented Nov 30, 2021 via email

cdaringe commented Dec 5, 2021

hhugo commented Dec 7, 2021

smorimoto commented Dec 7, 2021

TheSpyder commented Apr 26, 2022

TheSpyder commented Apr 26, 2022

feat(compiler): support target envs #1160

feat(compiler): support target envs #1160

Conversation

cdaringe commented Oct 19, 2021 • edited

Problem

Solution

hhugo commented Oct 19, 2021

hhugo commented Oct 19, 2021

cdaringe commented Oct 21, 2021 • edited

cdaringe commented Nov 2, 2021 • edited

cdaringe commented Nov 5, 2021 • edited

cdaringe commented Nov 11, 2021

cdaringe left a comment

Choose a reason for hiding this comment

cdaringe commented Nov 12, 2021

cdaringe commented Nov 21, 2021

hhugo Nov 29, 2021 • edited

Choose a reason for hiding this comment

hhugo commented Nov 29, 2021

hhugo commented Nov 29, 2021

hhugo commented Nov 30, 2021

cdaringe commented Nov 30, 2021 via email

cdaringe commented Dec 5, 2021

hhugo commented Dec 7, 2021

smorimoto commented Dec 7, 2021

TheSpyder commented Apr 26, 2022

TheSpyder commented Apr 26, 2022

cdaringe commented Oct 19, 2021 •

edited

cdaringe commented Oct 21, 2021 •

edited

cdaringe commented Nov 2, 2021 •

edited

cdaringe commented Nov 5, 2021 •

edited

hhugo Nov 29, 2021 •

edited