Skip to content

Wasm support#708

Draft
dfordivam wants to merge 22 commits into
developfrom
ob-wasm-integ-main-thread-3
Draft

Wasm support#708
dfordivam wants to merge 22 commits into
developfrom
ob-wasm-integ-main-thread-3

Conversation

@dfordivam
Copy link
Copy Markdown
Contributor

@dfordivam dfordivam commented Apr 20, 2020

Cleaned up (rebased) the work done for wasm support.

The feature is working fine now, though there are still nix related things to be fixed, especially in reflex-platform.

I have:

  • Based work on latest develop branch
  • Looked for lint in my changes with hlint . (lint found code you did not write can be left alone)
  • Run the test suite: $(nix-build -A selftest --no-out-link)
  • (Optional) Run CI tests locally: nix-build release.nix -A build.x86_64-linux --no-out-link (or x86_64-darwin on macOS)

@dfordivam dfordivam force-pushed the ob-wasm-integ-main-thread-3 branch from 03f1e81 to 1486cdb Compare April 21, 2020 06:06
@3noch
Copy link
Copy Markdown
Collaborator

3noch commented Apr 23, 2020

@dfordivam Does this obviate #702 ?

@dfordivam
Copy link
Copy Markdown
Contributor Author

@dfordivam Does this obviate #702 ?

Yes, and #689 also

Comment thread default.nix Outdated
Comment thread default.nix Outdated
, _backendConfig_staticAssets :: !StaticAssets -- ^ Static assets
, _backendConfig_ghcjsWidgets :: !(GhcjsWidgets (Text -> FrontendWidgetT (R frontendRoute) ()))
-- ^ Given the URL of all.js, return the widgets which are responsible for
, _backendConfig_ghcjsAssets :: !StaticAssets -- ^ Frontend ghcjs (and wasm) assets
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should rename this but it can be in a different PR.

Comment thread lib/backend/src/Obelisk/Backend.hs
Comment thread lib/backend/src/Obelisk/Backend.hs Outdated
Comment thread lib/backend/src/Obelisk/Backend.hs Outdated
Comment thread lib/backend/src/Obelisk/Backend.hs Outdated
Comment thread lib/backend/src/Obelisk/Backend.hs
Comment thread lib/backend/src/Obelisk/Backend.hs Outdated
Comment thread lib/backend/src/Obelisk/Backend.hs Outdated
Comment thread lib/backend/src/Obelisk/Backend.hs
Comment thread lib/backend/src/Obelisk/Backend.hs Outdated
Comment thread lib/backend/src/Obelisk/Backend.hs Outdated
Comment thread lib/backend/src/Obelisk/Backend.hs Outdated
Comment thread lib/backend/src/Obelisk/Backend.hs Outdated
Comment thread lib/backend/src/Obelisk/Backend.hs Outdated
Comment thread default.nix Outdated
shells-ghc = builtins.attrNames (self.predefinedPackages // self.shellPackages);

shells-ghcjs = [
shells-frontend = [
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ryan pointed out that it's possible to have a ghc frontend so my suggestion of shells-frontend isn't quite right. Maybe shells-web?

Comment thread lib/backend/src/Obelisk/Backend.hs Outdated
Comment thread lib/backend/src/Obelisk/Backend.hs Outdated
\}"

delayedJsScript n =
"var wasmFile = " <> wasmUrl <> ";\
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

var WASM_APP_URL or something and even a comment explaining what it's for.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There's enough "logic" in these drivers that it makes me want to templatize them so we only have to express the meaningful bits once.

Comment thread lib/backend/src/Obelisk/Backend.hs Outdated
\var tag = document.createElement('script');\
\tag.type = 'text/javascript';\
\tag.src = docSrc;\
\tag.setAttribute = ('defer', 'defer');\
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I fear for backwards compatibility on this syntax. Let's stick with the function call.

Suggested change
\tag.setAttribute = ('defer', 'defer');\
\tag.setAttribute('defer', 'defer');\

Comment thread lib/backend/src/Obelisk/Backend.hs Outdated
-- ^ (preload script, deferred run script, delayed run script (Int input is the delay))
wasmScripts allJsUrl' (wasmRoot, wAssets) = (preloadScript, runJsScript, delayedJsScript)
where
wrapName f = "'" <> wasmRoot <> "/" <> f wAssets <> "'"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since this stuff is user-supplied we should do at least basic sanitizing on it. Maybe replacing ' with \' at least.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't there some library (jmacro?) that actually knows how to do this properly? We should not be implementing escaping ourselves, and we certainly shouldn't be doing it inline in other code.

@dfordivam dfordivam force-pushed the ob-wasm-integ-main-thread-3 branch from c33fb9b to 2ff139a Compare May 8, 2020 06:04
@dfordivam dfordivam force-pushed the ob-wasm-integ-main-thread-3 branch from 2ff139a to 4534a5b Compare May 8, 2020 06:20
@bidigo
Copy link
Copy Markdown

bidigo commented Oct 8, 2020

Just a quick question: any idea if this is going to be merged anytime soon?

@dfordivam
Copy link
Copy Markdown
Contributor Author

@bidigo this will take a while. It needs more testing and performance improvements. There is a more recent work done on this branch https://github.com/obsidiansystems/obelisk/tree/dn-jsaddle-core2-2-wasm

@bidigo
Copy link
Copy Markdown

bidigo commented Oct 8, 2020

@dfordivam Thx for the info (and thanks for the great work!)

@mankyKitty mankyKitty marked this pull request as draft April 22, 2022 04:46
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.

4 participants