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

Refresh with Halogen, Tailwind, Webpack #192

Closed
wants to merge 3 commits into from

Conversation

milesfrain
Copy link
Contributor

@milesfrain milesfrain commented Aug 4, 2020

This PR aims to:

  • Trade JS and HTML code for PS
  • Improve type safety
    • The original version uses JQuery, which passes around lots of strings that aren't checked during compilation. This new version uses Halogen, which is a more type safe framework.
    • Tailwind improves type safety for CSS. It's also a great way to give PS code more control over styling.
  • Improve the developer workflow
    • Webpack dev server allows for fast incremental rebuilds and automatic page refresh.
  • Simplify iframe strategy
    • The original version involved bidirectional communication between iframe and parent, along with synchronization code (timeouts, retries, etc). This seems unnecessary. The new architecture allows the parent to simply dictate iframe content upon creation.

The core changes are in the last commit of this PR. The other commits are these PRs:
#190
#191

These PRs track additional features that may be added after this PR:
#193
#194
#195 (more for reference)

* Converted lots of HTML and JS code to PS
* Converted from JQuery to Halogen Hooks
* Using Tailwind CSS
* Using webpack for dev server and minification
@hdgarrood
Copy link
Collaborator

I'm really not keen on this, sorry. The UI needs of Try PureScript are not that complex and I would much rather have a simpler toolchain than add a bunch more moving parts and have to grapple with Webpack, even if it does mean we get things like automatic page refresh. I don't think our CSS suffers from a lack of type-safety, at least not enough to justify adding Tailwind (and a Python dependency for that Python script). It's also a little bit frustrating as a maintainer to receive a PR saying you've rewritten an existing component because the way it works currently seemed unnecessary to you; it's not clear that you've made much of an attempt to understand why it is the way it is currently or articulate what the concrete differences are. Any change to how the iframe works definitely warrants careful review and its own PR - we have to be very careful with this part of Try PS because we're running untrusted code inside the iframe.

@milesfrain
Copy link
Contributor Author

I can split these up further, but was hoping for some input on granularity and sequence. See #187 (comment) for one proposal.

I can explore other options for achieving automatic page refresh. I'd rather not use webpack either, but it's the only option I had success with. We could defer this feature until ES modules in 0.15 and then use snowpack.

Would Tailwind be more palatable if I converted the Python script to a .purs script?

I'll make a separate PR for the iframe changes so we can discuss that in more detail.

@natefaubion
Copy link
Contributor

natefaubion commented Aug 22, 2020

Would Tailwind be more palatable if I converted the Python script to a .purs script?

I think the main issue is why is it necessary? We should be able to accept the minimum possible changes in order to achieve a specific feature or goal. Webpack, Halogen, Tailwind, etc are not necessary at all in order to get gist saving or any of the other features. I understand you may think these are all better than the status quo, so it would be beneficial to accept them, but they are arbitrary. I'm not super keen on JQuery either, but the question is whether you want to see these features merged, or if you want to push this specific architecture.

I personally think that we should be able to develop and build this with stock purs tooling.

The original version involved bidirectional communication between iframe and parent, along with synchronization code (timeouts, retries, etc). This seems unnecessary. The new architecture allows the parent to simply dictate iframe content upon creation.

Can you explain why you think it's unnecessary?

@milesfrain
Copy link
Contributor Author

Can you explain why you think it's unnecessary?

I think 1-way communication is a lot simpler than 2-way communication with synchronization. The live prototype demonstrates that 1-way communication is all we need.

Similar to how setting-up a <div> containing some text would be a lot more complex if you had to go through a similar process as our existing iframe strategy:

  • Each div contains a listener waiting for a message containing some text to display
  • Parent creates a div
  • Parent makes multiple attempts to send text message to div
  • Div receives text message and sends a response
  • Parent receives response from div and stops making further attempts to send text to div

It's nicer to just use this approach:

  • Parent creates div with the text it should contain

The new iframe changes enable this latter approach.

@natefaubion
Copy link
Contributor

natefaubion commented Aug 22, 2020

It is very difficult for me to assess any potential effects of that change because it is included in such a large PR with many other changes. I want to make it clear that we aren't just trying to be obstinate, it is a matter of PR etiquette. Some of this is just hard, and we don't have everything in our head at all times. I wrote the iframe code about two years ago, but I know it was not arbitrary and I also don't remember every detail about it off the top of my head. I'd need to look at the differences side-by-side, but that's extremely difficult to do while also considering so many other changes. We are not under an obligation to just trust what you've written and accept your architectural decisions because you put a lot of time into it. If you started implementing a feature for a JS project by rewriting it in PureScript, I think it is unlikely that a maintainer would accept it, and this is not much different, just with tools and architecture. None of your decisions are necessarily wrong or incorrect, but it's just too much to consider at one time.

@milesfrain
Copy link
Contributor Author

I totally understand that big PRs are more difficult to review, and will work with maintainers to split this up as requested. I just didn't want to make guesses on how this should be split up ahead of time and waste additional time on that.

I personally think that we should be able to develop and build this with stock purs tooling.

The development loop with stock tooling is really painful (15 seconds to observe changes). Opened an issues for side discussion on that in #197

@hdgarrood
Copy link
Collaborator

Here are some changes I’d be happy to see and review as individual PRs:

  • Enable saving gists
  • Convert ContT Effect to Aff
  • Store code compressed in the URL instead of using local storage

I agree with @natefaubion on stock tooling. I appreciate that this might be a bit disappointing to hear but I would rather deal with the developer experience issue by getting ES modules shipped, as that will allow us to not have a bundling step during development at all.

@thomashoneyman
Copy link
Member

This was mostly done in #215, with iframe changes done in #275.

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