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

Missing primitives in rely with JSOO #207

Open
Et7f3 opened this issue Nov 5, 2019 · 4 comments
Open

Missing primitives in rely with JSOO #207

Et7f3 opened this issue Nov 5, 2019 · 4 comments
Assignees

Comments

@Et7f3
Copy link

Et7f3 commented Nov 5, 2019

Hy I wanted to update https://github.com/bryphe/reason-gl-matrix to rely. I was able to convert to rely natively but I can't run when building with JSOO.

I have some repro on my fork on branch Et7f3/test/switch_to_rely or commit instead Et7f3/reason-gl-matrix@ec2fd9d

I quote some message from discord (https://ptb.discordapp.com/channels/235176658175262720/469417973820686336/641021659989540925).

Me:

For info it run fine on native with dune. I have tried only with a empty file that include Rely and I get some strange error maybe linked to str
Image that show the error.

@bandersongit

You can also look at https://github.com/aweis/irmin-web as a currently working example (though it does appear to be pinned to an older version of Rely)

Me:

I have seen in irmin-web they have added --profile=release flag. When I add it I get some other error.
Show missing primitives error.

@bandersongit

Looks like the issue with re is solveable by releasing a new version of Pastel

Pastel version should do it, got it working with a link to current master
Fix with the new pastel

also had to add a stub for unix_isatty for pastel,
I used

//Provides: unix_isatty

function unix_isatty (f) {
    return false;
};

I also changed your Testramework to

module MockSnapshotIO = {
  let readSnapshotNames = _ => [];
  let readFile: string => string = s => s;
  let removeFile: string => unit = _ => ();
  let writeFile: (string, string) => unit = (_, _) => ();
  let existsFile: string => bool = _ => false;
  let mkdirp = _ => ();
};
/* Snapshots will be virtual-modulified at some point, until then we need to inject
 * something custom with RelyInternal to make JSOO happy because the default snapshot
 * implementation uses the Unix module  */
include RelyInternal.TestFramework.MakeInternal(
          MockSnapshotIO,
          {
            let config =
              RelyInternal.TestFrameworkConfig.TestFrameworkConfig.initialize({
                snapshotDir: "test/__snapshots__",
                projectDir: "",
              });
          },
        );
@bandersongit bandersongit self-assigned this Nov 5, 2019
facebook-github-bot pushed a commit that referenced this issue Nov 6, 2019
…ply to be prefixed by unstable to reflect their currently experimental nature (#208)

Summary:
We need to release a new version of Pastel for JSOO compatibility (#207). This should ideally have been done when the usage of Str was removed in favor of Re
Pull Request resolved: #208

Differential Revision: D18341628

Pulled By: bandersongit

fbshipit-source-id: 05fb76a8aa19a7d7234d4f9bbf54be33d5bbe351
@bandersongit
Copy link
Contributor

@Et7f3 A new version of Pastel has been released. I'll spend some time in the next couple days making it so that snapshots don't cause these errors if they aren't being used.

@bandersongit
Copy link
Contributor

I filed ocsigen/js_of_ocaml#913 to add unix_is_atty to JSOO along with my thoughts on implementation.

I've verified that your example now works if you provide a manual stub for unix_isatty, use the latest version of Pastel, and create a snapshot directory.

@Et7f3
Copy link
Author

Et7f3 commented Nov 10, 2019

I'm blind I don't see where you fix str issue 👀.

@bandersongit
Copy link
Contributor

bandersongit commented Nov 20, 2019

I'm blind I don't see where you fix str issue 👀.

@Et7f3 I removed references to str here d3203ea, we just hadn't cut a Pastel release since then

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

No branches or pull requests

2 participants