Skip to content

Update to 0.15.0; use ES Module Shims #275

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

Merged
merged 35 commits into from
Jun 8, 2022
Merged

Update to 0.15.0; use ES Module Shims #275

merged 35 commits into from
Jun 8, 2022

Conversation

JordanMartinez
Copy link
Contributor

@JordanMartinez JordanMartinez commented May 19, 2022

Description of the change

Fixes #274 and fixes #263. Another take that succeeds #274.

Original opening thread

In #274, I experienced a few issues that made me rethink how I added initial support for 0.15.0. I can explain in a future comment because it's late right now.

Regardless, this PR is currently stuck due to this error:

Uncaught (in promise) DOMException: Permission denied to access property "_$s" on cross-origin object srcdoc:1

Clicking on srcdoc:1 leads to this message:

Error loading this URI: Unknown source

I'm baffled on how to proceed. I've been using Firefox to test this out, so it may be related to https://bugzilla.mozilla.org/show_bug.cgi?id=1721891.

When I tried it on Chromium, the error I get is this:

Uncaught (in promise) DOMException: Blocked a frame with origin "null" from accessing a cross-origin frame.
at about:srcdoc:1:163

Clicking on srcdoc:1:163 displays No resource with given URL found.

Changes made:

  • We're using [email protected] instead of 1.5.1 doesn't load the modules correctly.
  • Since the output of the compiler is ../Module.Path/index.js, we have to remap the import paths to Config.url <> "/Module.Path/index.js"
  • We use window.runCount to ensure main is only called once. Otherwise, it will be called an infinite number of times

Checklist:

  • Added the change to the changelog's "Unreleased" section with a reference to this PR (e.g. "- Made a change (#0 by @)")
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

@JordanMartinez
Copy link
Contributor Author

Turns out the above error was a false alarm. We use index.html to create an iframe using frame.html as the source. es-module-shims appears to create an iframe within the head of frame.html. That's the source of the above error. I'm not sure why it does that, but it distracted me.

The actual reason why the code wasn't working is because there wasn't a wrapper calling main(); in the script's text. Adding that makes the example appear.

@JordanMartinez
Copy link
Contributor Author

And I think the above error was fixed in a more recent version of es-module-shims. I was using 1.5.1, but it no longer appears in 1.5.5.

@mikesol
Copy link

mikesol commented May 19, 2022

Going through this now. I was able to get files served by running npx http-server --cors (we should probably update the readme, as it currently recommends npx http-server).

After that, I'm getting:

VM60 about:srcdoc:1 Uncaught (in promise) DOMException: Blocked a frame with origin "null" from accessing a cross-origin frame.
    at about:srcdoc:1:163

Is that where you're at as well?

@JordanMartinez
Copy link
Contributor Author

Going through this now. I was able to get files served by running npx http-server --cors (we should probably update the readme, as it currently recommends npx http-server).

After that, I'm getting:

VM60 about:srcdoc:1 Uncaught (in promise) DOMException: Blocked a frame with origin "null" from accessing a cross-origin frame.
    at about:srcdoc:1:163

Is that where you're at as well?

See the package.json file. If you run npm i && npm run serve:dev, it'll run the code properly. The DOMException is what I get, too, but it's something to ignore. On my side, the code does get run if I'm not using the shims.

AFAICT, the shims don't currently work because they aren't imported in the code we're using. Put differently, this should work:

import React from 'react';
..

but this doesn't work

// main import script
import * as React from "../React/index.js";
...

where React/index.js is:

import React from 'react';
...

I think the solution there are two possible solutions:

  • add another import map for all packages in the package set to frame.html
  • use esbuild to bundle the outputted JS and return it in the /compile API endpoint

For now, I'm going with the esbuild option since that seems to be what #264 is for.

@mikesol
Copy link

mikesol commented May 19, 2022

Would it be the case, then, that esbuild bundles and sends the whole application with each call to compile?

@natefaubion
Copy link
Contributor

natefaubion commented May 19, 2022

AFAICT, the shims don't currently work because they aren't imported in the code we're using. Put differently, this should work:

What convinced you that this is the case? It's not clear to me whether the imports are transitive should matter. Is this documented?

@JordanMartinez
Copy link
Contributor Author

Code now works, but the main function gets called infinitely.

@natefaubion
Copy link
Contributor

Doing a full bundle on every compile is extremely expensive, both on the server and for the user to pull down. I personally think it's a non-starter.

@natefaubion
Copy link
Contributor

I'm pretty sure #264 is for only bundling the UI, not in general for all code the user inputs.

@natefaubion
Copy link
Contributor

natefaubion commented May 19, 2022

add another import map for all packages in the package set to frame.html

I don't think you need to add an import map for all packages in the package set? You only need it for specific packages that import specific foreign modules. This seems fine to me, and more preferable than using esbuild.

@JordanMartinez
Copy link
Contributor Author

AFAICT, the shims don't currently work because they aren't imported in the code we're using. Put differently, this should work:

What convinced you that this is the case? It's not clear to me whether the imports are transitive should matter. Is this documented?

Let's say I want to run the following code that uses the decimal.js shim:

module Main where

import Prelude

import Data.Foldable (fold)
import Effect (Effect)
import Effect.Console (log)
import TryPureScript as TP
import Data.Decimal as D

main :: Effect Unit
main = TP.render =<< TP.withConsole do
  log $ show $ D.fromString "2424.24245238523752384034"

The unbundled JS returned is:

import * as Control_Bind from "../Control.Bind/index.js";
import * as Data_Decimal from "../Data.Decimal/index.js";
import * as Data_Maybe from "../Data.Maybe/index.js";
import * as Data_Show from "../Data.Show/index.js";
import * as Effect from "../Effect/index.js";
import * as Effect_Console from "../Effect.Console/index.js";
import * as TryPureScript from "../TryPureScript/index.js";
var main = /* #__PURE__ */ Control_Bind.bindFlipped(Effect.bindEffect)(TryPureScript.render)(/* #__PURE__ */ TryPureScript.withConsole(/* #__PURE__ */ Effect_Console.log(/* #__PURE__ */ Data_Show.show(/* #__PURE__ */ Data_Maybe.showMaybe(Data_Decimal.showDecimal))(/* #__PURE__ */ Data_Decimal.fromString("2424.24245238523752384034")))));
export {
    main
};

// I add this part so the code actually runs.
main();

If I remap those imports from ../Module.Path/index.js to ./js/output/Module.Path/index.js, and then update frame.html to the following:

<!DOCTYPE HTML>
<html>
<head>
  <title>Try PureScript!</title>
  <meta content="text/html;charset=utf-8" http-equiv="Content-Type">
  <meta content="utf-8" http-equiv="encoding">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <script>
    window.esmsInitOptions = {
      // -- Hooks --
      // Module load error
      onerror: (e) => {
        console.log("Error while loading module: ");
        console.log(e);
        throw e;
      },
    };
  </script>
  <!--
    JSPM Generator Import Map
    Edit URL: https://generator.jspm.io/#Y2NnYGCzD80rySzJSU1hSMpM183MK0lNTy1yMNQz0zM1ZEhJTc7MTczRyyp2MDTQM9YzZChKTUwu0U3Jz3UwNNMzxCqiX5xaVJZaBJGAKystzUxxsACaYQQAoBlP83cA
  -->
  <script type="importmap">
  {
    "imports": {
      "big-integer": "https://ga.jspm.io/npm:[email protected]/BigInteger.js",
      "decimal.js": "https://ga.jspm.io/npm:[email protected]/decimal.js",
      "react": "https://ga.jspm.io/npm:[email protected]/index.js",
      "react-dom": "https://ga.jspm.io/npm:[email protected]/index.js",
      "react-dom/server": "https://ga.jspm.io/npm:[email protected]/server.browser.js",
      "uuid": "https://ga.jspm.io/npm:[email protected]/dist/esm-browser/index.js"
    },
    "scopes": {
      "https://ga.jspm.io/": {
        "object-assign": "https://ga.jspm.io/npm:[email protected]/index.js",
        "react": "https://ga.jspm.io/npm:[email protected]/index.js",
        "scheduler": "https://ga.jspm.io/npm:[email protected]/index.js"
      }
    }
  }
  </script>

  <!-- ES Module Shims: Import maps polyfill for modules browsers without import maps support (all except Chrome 89+) -->
  <script async src="https://ga.jspm.io/npm:[email protected]/dist/es-module-shims.js" integrity="sha384-ZGtUNdKMtA2sSrO9dN9d2TOeiSlXlDSrOrasgO9YoKR4LsDV7RLdUvvX1M0gCkl2" crossorigin="anonymous"></script>

  <link rel="modulepreload" href="https://ga.jspm.io/npm:[email protected]/BigInteger.js" integrity="sha384-Lxn1K4ox8//F5O+uihYrTIJvbfM+UJaRL2SdFA2s/z0v6i5TrWg9B8RzfjM60R9y"/>
  <link rel="modulepreload" href="https://ga.jspm.io/npm:[email protected]/decimal.js" integrity="sha384-camBDJB1KoavtyRdQfdzzR4mj+uAKnNlvseJIwqKYOaHWnjZmnXR+1vyBP1WlZd0"/>
  <link rel="modulepreload" href="https://ga.jspm.io/npm:[email protected]/index.js" integrity="sha384-iQp1zoaqIhfUYyYkz3UNk1QeFfmBGgt1Ojq0kZD5Prql1g7fgJVzVgsjDoR65lv8"/>
  <link rel="modulepreload" href="https://ga.jspm.io/npm:[email protected]/cjs/react-dom-server.browser.production.min.js" integrity="sha384-XL5bjsaJ5UPjTIPsyzUfscq41OFbVXsLojl53DCgFfdzKNNUL2MOSd0WM/upjd53"/>
  <link rel="modulepreload" href="https://ga.jspm.io/npm:[email protected]/cjs/react-dom.production.min.js" integrity="sha384-HDQjgsF+F2j5XuiiDtCiuLA1vgyHr9ONiNrBhKmJEVa43KqIZdAB7VN2+QjUGwvq"/>
  <link rel="modulepreload" href="https://ga.jspm.io/npm:[email protected]/index.js" integrity="sha384-zAu0L7n/xnRAG7+D/qd1K4E1/2tYK4hL3puH/5YEliHNnO3OOt5jUhnnemFTHmIG"/>
  <link rel="modulepreload" href="https://ga.jspm.io/npm:[email protected]/server.browser.js" integrity="sha384-dsW+4a3rjWELxqtR1rVEIeLNHGyMjRFwdBVz6OG30oBLs8tO2/dYUIxEvc515PSZ"/>
  <link rel="modulepreload" href="https://ga.jspm.io/npm:[email protected]/index.js" integrity="sha384-T0mNrKFgmKBye+m7XrpShrh+7kfix2zQd9qrR8xytnppcxESpgzrb4IJXvE+uJcl"/>
  <link rel="modulepreload" href="https://ga.jspm.io/npm:[email protected]/cjs/react.production.min.js" integrity="sha384-pTMZhybzHZ+1G029kWUmoGvTrBp1C+2oJAkZV48BBq7+e6Hk3bGuXtvxT2vQfBqj"/>
  <link rel="modulepreload" href="https://ga.jspm.io/npm:[email protected]/index.js" integrity="sha384-jVagjV+2YtlseazU2byX6gMLPHaA5Ps2c6HhvsGDlWjn45YCCoU1q+QtQTOb1MjT"/>
  <link rel="modulepreload" href="https://ga.jspm.io/npm:[email protected]/index.js" integrity="sha384-HIW1B3OQdGPkUEgh29MiYoUKQp+mFTjw50hRVvGhZgUaNYrUFYN07a+3CqC0/I7L"/>
  <link rel="modulepreload" href="https://ga.jspm.io/npm:[email protected]/dist/esm-browser/index.js" integrity="sha384-y7UmGnwe+/V/S4AD5T6MLU9dHjA3PLvuVGNhrqZApiNbq9Lf8s3aNRrcnzOa+qtR"/>
-  <script src="js/frame.js"></script>
</head>
<body>
  <main id="main"></main>
+  <script>
+    import * as Control_Bind from "./js/outpControl.Bind/index.js";
+    import * as Data_Decimal from "./js/outpData.Decimal/index.js";
+    import * as Data_Maybe from "./js/outpData.Maybe/index.js";
+    import * as Data_Show from "./js/outpData.Show/index.js";
+    import * as Effect from "./js/outpEffect/index.js";
+    import * as Effect_Console from "./js/outpEffect.Console/index.js";
+    import * as TryPureScript from "./js/outpTryPureScript/index.js";
+    var main = /* #__PURE__ */ Control_Bind.bindFlipped(Effect.bindEffect)(TryPureScript.render)(/* #__PURE__ */ TryPureScript.withConsole(/* #__PURE__ */ Effect_Console.log(/* #__PURE__ */ Data_Show.show(/* #__PURE__ */ Data_Maybe.showMaybe(Data_Decimal.showDecimal))(/* #__PURE__ */ Data_Decimal.fromString("2424.24245238523752384034")))));
+    export {
+        main
+    };
+    main();
  </script>
</body>
</html>

I get the following error in the console:

Uncaught SyntaxError: import declarations may only appear at top level of a module

and the position of the error is the first import:

import * as Control_Bind from "./js/outpControl.Bind/index.js";

You only need it for specific packages that import specific foreign modules.

If only a few need to be in the import map, that would be preferable than creating an import map for the entire package set.

@natefaubion
Copy link
Contributor

For modules you need <script type="module">. I wonder if we can avoid rewriting paths by using a <base> tag.

@JordanMartinez
Copy link
Contributor Author

JordanMartinez commented May 19, 2022

For modules you need <script type="module">.

🤦 Wow... That change fixed it. And the import maps didn't need to be updated.

EDIT: actually, setting the script's type to module was what I was already doing in the frame.js code. Hm...

@JordanMartinez JordanMartinez marked this pull request as ready for review May 19, 2022 23:35
@JordanMartinez JordanMartinez mentioned this pull request May 19, 2022
4 tasks
@JordanMartinez
Copy link
Contributor Author

This is ready for a final review. Code builds and ES modules work.

@natefaubion
Copy link
Contributor

Not quite. Since this is used in both dev (./js/output/ignored) and production settings , hard-coding it for dev breaks it for production.

Sorry can you clarify? dev and production use different path schemes? Is that necessary? If so, is there a way to configure it at build time, or app launch time (not necessarily client-side)? If there is a reasonable way to avoid a search and replace, that would be nice.

@JordanMartinez
Copy link
Contributor Author

Sorry can you clarify? dev and production use different path schemes? Is that necessary? If so, is there a way to configure it at build time, or app launch time (not necessarily client-side)? If there is a reasonable way to avoid a search and replace, that would be nice.

We sort of already configure it at build time via the Config.purs file:

If we wanted to do it without a search and replace approach and use the base element, then we'd need to overwrite that value. Otherwise, it would require duplicating the frame.html file.

I've pushed a commit that uses an NPM script to overwrite it to the correct values.

.map((line) => line.replace(/^( *<base href=")[^"]*(".+)$/, `$1${baseHref}$2`))
.join("\n");

fs.writeFileSync(filePath, newHtml);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have to rewrite the file in place, I'm not sure this is the best way to do this. Will this dirty the source tree?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also curious how this works with the actual prod deployment of the UI. I'm not fully aware of all that goes into that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it would dirty the source tree.

The client is built in the ci.yml file by using NPM scripts before packaging up the results. When we deploy the code, the server downloads the client.tar.gz, unpacks it, and then runs it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should this issue be resolved? Do we create 2 versions of the frame.html file, one with base set to one and another with base set to something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a check in CI to fail the build if the base element does not refer to the production path. One can use npm run base:dev to test out their code in development and then run npm run base:production before committing the final results. This ensures the following:

  • local dev still works without much friction in workflow
  • the source tree is clean when the production build is made
  • frame.html isn't duplicated, so there's only one place to update the import maps

I believe that resolves your final issue with this PR.

Copy link

@mikesol mikesol left a comment

Choose a reason for hiding this comment

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

Amazing work! One tiny comment, otherwise 🚀

@JordanMartinez
Copy link
Contributor Author

Can this PR get an approval? I believe I've addressed all feedback.

Copy link
Contributor

@MonoidMusician MonoidMusician left a comment

Choose a reason for hiding this comment

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

I was able to build and run locally, and I verified that the linked examples plus a Halogen demo of my own still work.

@JordanMartinez JordanMartinez merged commit 0758325 into purescript:master Jun 8, 2022
@JordanMartinez JordanMartinez deleted the up-to-15-with-esm branch June 8, 2022 18:58
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.

Proposal: Load foreign code with ES Module Shims and import maps
4 participants