Enable gist saving plus other updates #187

milesfrain · 2020-08-02T21:08:46Z

Live demo at https://try.ps.ai

Now we can launch cookbook recipes directly in the browser.

PRs and Issues addressed:

Unable to create a gist #118
Allow loading examples from GitHub #181
Allow any module name #182 - Simply overwrites the module name client-side. The backend solution proposed in that PR seems more robust though. Example: https://try.ps.ai/?github=JordanMartinez/purescript-cookbook/master/recipes/ButtonsHalogenHooks/src/Main.purs

New features:

Editor state tracked in sharable url, rather than local storage
- URL sharing should always work as expected now
Gist saving
Load files from github repo paths
Compiles any module name
- Simply renames to module Main in editor

Code quality improvements:

Traded a bunch of JS and HTML code for PS and ecosystem libraries
Aff instead of ContT

Questionable design choices:

Converted from JQuery to Halogen Hooks
Using Tailwind for CSS
Different IFrame strategy
- No bidirectional communication with parent.
- No click handlers for links to gists or files. Links behave the same as they would on any other page.
- Kinda hacky solution. Keeping in the theme of sending strings to be executed as JS code.
Using webpack for dev server and production bundling
- I'm not a webpack fan, but it's the only tool that I was able to get working for this project.
I'd like to rename the Try module prefix to TPS, to distance from try-catch.
- Holding off on this for now for clearer diffs during review.

Minor changes:

Mobile breakpoint changed from 720 to 640 px
Using default browser font instead of roboto
Favicon converted to SVG (with transparent .ico fallback), which looks nicer in non-active tab
- Old:
- New:

Minor bugs shared with original version:

Ace editor markers (red underline) don't follow text
- Improving this behavior does not seem worthwhile, since it's only an issue until the next recompile.
Selecting "code" for View Mode, then shrinking browser width below mobile breakpoint, results in no view.
- Does not seems like a significant problem, and any attempts to change this behavior might be more annoying for users.

Missing features:

query params for view-mode, auto-compile, show-js
- This feature did not behave as expected for me in the original app:
  - These settings are not updated in the url when toggled
  - Even with compile=false there's still one compilation on load
- I can spend more time restoring this feature, but am wondering:
  - What is the intended use case for these, and does anyone use it?
  - I'd need some guidance on how to handle routing with these optional query parameters.
- I don't see a straightforward way to preserve these settings through a GH OAuth callback

Integration next steps:

Create another GitHub OAuth app managed by PS org
- Demo currently uses my OAuth app
Migrate OAuth secret-keeping app to PS org
- Demo currently a serverless function on my Azure account - source code and instructions here https://github.com/milesfrain/gist-prototype/tree/master/azure
Re-integrate CI

Other todos:

Refine documentation and code comments
Update PS dependencies (ace / halogen) with the temporary fixes in this project

* Converted lots of HTML and JS code to PS * Converted from JQuery to Halogen Hooks * Using Tailwind CSS * Enabled gist saving * Enabled loading files from github repo paths * Editor state tracked in shareable url, rather than local storage

milesfrain · 2020-08-03T00:25:16Z

client/public/index.html

-    editor.renderer.setShowGutter(true);
-    editor.setFontSize(13);
-    editor.setShowPrintMargin(false);
-
-    var session = editor.getSession();
-
-    session.setMode('ace/mode/haskell');
-    session.setOptions({
-      tabSize: 2,
-      useSoftTabs: true
-    });


showGutter already defaults to true

original font size is 13, but appears to default to 12. Can't edit this without fixing ace editor library to take a number rather than a string for font size.

soft tabs enabled by default

milesfrain · 2020-08-03T00:26:12Z

client/public/index.html

-  <script type="text/javascript" src="//cdnjs.cloudflare.com/ajax/libs/jquery/1.12.4/jquery.js"></script>
-  <script type="text/javascript" src="//cdnjs.cloudflare.com/ajax/libs/ace/1.1.01/ace.js" charset="utf-8"></script>
-  <script type="text/javascript" src="//cdnjs.cloudflare.com/ajax/libs/ace/1.1.01/mode-haskell.js"></script>
-  <script type="text/javascript" src="//cdnjs.cloudflare.com/ajax/libs/ace/1.1.01/theme-dawn.js"></script>


theme-dawn does not appear to be used, so I removed it.

milesfrain · 2020-08-03T00:27:14Z

client/public/index.html

-  <link rel="stylesheet" type="text/css" href="//fonts.googleapis.com/css?family=Roboto:300,600">
-  <link rel="stylesheet" type="text/css" href="//fonts.googleapis.com/css?family=Roboto+Slab:300,600">


Is this font necessary? I think the default font looks pretty nice in the demo app.

milesfrain · 2020-08-03T00:28:50Z

client/package.json

@@ -1,17 +1,56 @@
 {
-  "name": "trypurescript-client",
-  "private": true,
+  "name": "tps",


Renamed from trypurescript-client to make changing configs with these commands a bit less verbose:

npm config set tps:configpath "config/dev/*.purs" npm config set tps:configpath "config/prod/*.purs"

milesfrain · 2020-08-03T00:31:17Z

client/package.json

+
+    "c1-lock-css": "# -- Strip away unused css from autogenerated purs files.",
+    "c2-lock-css": "# -- This improves rebuild times, but won't allow adding new css.",
+    "c3-lock-css": "# -- So re-run gen-css before making additional css changes.",
+    "lock-css": "npm run bundle && npm run css-purge && npm run css2purs",
+


It turns out that npm i will reformat this file and remove the unnecessary newlines I added for readability. Really terrible that you can't add comments to json files without workaround like this. Would like to migrate to something better if it exists.

milesfrain · 2020-08-03T00:37:50Z

client/src/Try/Component.purs

+            -- Compress source file contents.
+            -- This avoids non-trivial HTML + JSON string escaping
+            let
+              objcomp = map (unwrap >>> compressToEncodedURIComponent) obj
+            -- Convert object to JSON string
+            let
+              jsonStr = stringify $ encodeJson objcomp
+            -- Pass JS files to execute in iframe
+            putOutput $ App jsonStr $ JS js


The JS files are compressed with lz-string because I couldn't figure out how to escape the contents correctly to pass to the iframe. It would be nice to escape these a bit more efficiently, although I don't think this compression/decompression step is a major performance issue.

milesfrain · 2020-08-03T00:54:29Z

client/src/Try/Common.purs

+--
+-- Page to launch on startup and when clicking home button
+homeRoute :: String
+homeRoute = "/?github=milesfrain/tps/demo/examples/Home.purs"


Considering changing these paths to expect the blob part so they're easier for users to copy while browsing a repo. Then it just needs to be stripped out from the raw url for loading.

https://github.com /milesfrain/tps/blob/demo/examples/Home.purs https://raw.githubusercontent.com/milesfrain/tps/ demo/examples/Home.purs

milesfrain · 2020-08-03T00:58:58Z

client/src/Try/Loader.purs

+              case res of
+                Left err -> pure { name, path, deps: [], src }
+                  where
+                  src = throwJSError $ "Could not get shim " <> name <> " at " <> shim.url <> ", " <> AX.printError err


This should point to a guide on adding new shims

milesfrain · 2020-08-03T01:25:09Z

client/src/Try/Loader.purs

  moduleCache :: Ref (Object Module)
  moduleCache = unsafePerformEffect (Ref.new Object.empty)


I was hoping there would be a nice way to do this with memoize, but I couldn't figure it out. It doesn't seem like memoize can be used to bypass repeating effects (such as http requests in this case).

hdgarrood · 2020-08-03T16:40:02Z

This is very difficult to review as-is because of its size. Can you please break this up into smaller PRs which address individual points? Also can you please back out the changes which already exist in other PRs? It's better to just get those changes in one at a time.

thomashoneyman · 2020-08-03T18:35:53Z

Thanks for doing this, @milesfrain! I'd love to review this and I'm happy to see some improvements coming in. As @hdgarrood said, it's a little difficult to review a PR of this size, and it would be much easier to review and faster to merge PRs for individual points if it's possible to break this up.

milesfrain · 2020-08-03T19:03:31Z

I agree that a giant wave of changes is not ideal.

I can back out these feature additions:

Gist saving
File loading
Any module name

But splitting-apart the remaining changes would involve a lot of throw-away effort to reach a working state between each PR and would be more code to review overall.

The only code that ended up being reused across the rewrite is:

src/Try/Loader.purs
src/Try/Types.purs
src/Try/Shim.purs
config/*/Try.Config.purs
src/Try/API.* is now src/Try/Compile.purs
src/Try/Gist.* is now src/Try/Request.purs
public/js/frame.js is now public/frame-load.js

The code may be easiest to review by considering it as a full rewrite and exploring it as a fresh new project locally, rather than as an incremental change in the diff viewer. It's 1400 lines of commented PS code.

No rush on getting this merged, since these features are available in the live demo, and I'm happy to use that in the meantime. I also don't want to take attention away from other tasks. I'll still do my best to help with the merge effort - starting with making another PR containing fewer new features.

hdgarrood · 2020-08-03T19:14:11Z

I would rather have more code to review overall if that allows making changes incrementally; I don't want to do a full rewrite. Most of the individual changes you've listed do make the most sense to do incrementally, in my mind. Also, I don't want to make changes as far-reaching as moving from jquery to halogen hooks, or moving to tailwind css, or changing the iframe strategy, without a proper justification. I think each of these things really needs its own issue to make the case for why it's a good idea.

milesfrain · 2020-08-04T21:37:20Z

I split this up into six PRs: #190 though #195.

#192 is still pretty significant.

If this needs to be broken-up further, we should first agree on a commit sequence. This is one option:

webpack
iframe
jquery -> halogen
- and saving state in URL
tailwind css

But there's a pretty significant time cost to doing this, so I don't want to dive into that unless it's the only way to move forward with incorporating these changes.

milesfrain added 3 commits August 2, 2020 12:44

add homepage example

a2973c5

TryPureScript refresh

292de50

* Converted lots of HTML and JS code to PS * Converted from JQuery to Halogen Hooks * Using Tailwind CSS * Enabled gist saving * Enabled loading files from github repo paths * Editor state tracked in shareable url, rather than local storage

rename TPS back to Try for easier diffs

e5d748c

milesfrain marked this pull request as draft August 2, 2020 21:46

reorganize and remove unused code

c2201d2

milesfrain commented Aug 3, 2020

View reviewed changes

minor package.json typo

f0fdd01

milesfrain commented Aug 3, 2020

View reviewed changes

milesfrain mentioned this pull request Aug 3, 2020

Add (try) links to Try PureScript demo for web recipes JordanMartinez/purescript-cookbook#195

Merged

milesfrain marked this pull request as ready for review August 3, 2020 01:20

milesfrain commented Aug 3, 2020

View reviewed changes

This was referenced Aug 22, 2020

Refresh with Halogen, Tailwind, Webpack #192

Closed

Enable faster development loop #197

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable gist saving plus other updates #187

Enable gist saving plus other updates #187

milesfrain commented Aug 2, 2020 •

edited

Loading

milesfrain Aug 3, 2020

milesfrain Aug 3, 2020

milesfrain Aug 3, 2020

milesfrain Aug 3, 2020

milesfrain Aug 3, 2020

milesfrain Aug 3, 2020

milesfrain Aug 3, 2020

milesfrain Aug 3, 2020

milesfrain Aug 3, 2020

hdgarrood commented Aug 3, 2020

thomashoneyman commented Aug 3, 2020

milesfrain commented Aug 3, 2020

hdgarrood commented Aug 3, 2020

milesfrain commented Aug 4, 2020

		<link rel="stylesheet" type="text/css" href="//fonts.googleapis.com/css?family=Roboto:300,600">
		<link rel="stylesheet" type="text/css" href="//fonts.googleapis.com/css?family=Roboto+Slab:300,600">

		moduleCache :: Ref (Object Module)
		moduleCache = unsafePerformEffect (Ref.new Object.empty)

Enable gist saving plus other updates #187

Are you sure you want to change the base?

Enable gist saving plus other updates #187

Conversation

milesfrain commented Aug 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hdgarrood commented Aug 3, 2020

thomashoneyman commented Aug 3, 2020

milesfrain commented Aug 3, 2020

hdgarrood commented Aug 3, 2020

milesfrain commented Aug 4, 2020

milesfrain commented Aug 2, 2020 •

edited

Loading