trypurescript icon indicating copy to clipboard operation
trypurescript copied to clipboard

Enable gist saving plus other updates

Open milesfrain opened this issue 5 years ago • 5 comments

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

Now we can launch cookbook recipes directly in the browser.

PRs and Issues addressed:

  • #118
  • #181
  • #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: image image
    • New: image image

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

milesfrain avatar Aug 02 '20 21:08 milesfrain

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.

hdgarrood avatar Aug 03 '20 16:08 hdgarrood

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.

thomashoneyman avatar Aug 03 '20 18:08 thomashoneyman

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.

milesfrain avatar Aug 03 '20 19:08 milesfrain

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.

hdgarrood avatar Aug 03 '20 19:08 hdgarrood

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:

  1. webpack
  2. iframe
  3. jquery -> halogen
    • and saving state in URL
  4. 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 avatar Aug 04 '20 21:08 milesfrain