konnorrogers / mrujs Goto Github PK

Currently we only advance. Not sure when to replace other than when a user sets it.

https://github.com/turbolinks/turbolinks#application-visits

Describe Problem

Although I don't want to entirely break the current behavior, I think @seanpdoyle added a number of key points above best practices that I think would be prudent of mrujs to document. For example, using <button> over <a> for data-method and using formmethod and formaction instead of data-method and data-url respectively.

Provide Examples

hotwired/turbo-rails#257 (comment)

The mutationObserver now properly checks for changed attributes, but what happens if attributes are removed? Should they be rechecked for their validity and have their event listeners removed? If so, how do we make this happen? Do we create a Set of all watched nodes and run through them and make sure theyre correct? this is an...interesting...problem to say the least.

Perhaps another filter could be added to the src/utils/dom.ts to check the validity of the node[s] whos attributes were removed?

Additionally, morphs should be enable-able / disable-able on remote forms / links. data-ujs-morph="true" and data-mrujs-morph="true" should both be supported.

rails/rails#43031

Expected Behaviour

no error

Actual Behaviour

Uncaught TypeError: Cannot redefine property: submitter
    at Function.defineProperty (<anonymous>)
    at turbo.es2017-esm.js:39

Steps to Reproduce

in application.js

import mrujs from "mrujs"
import { Turbo } from "@hotwired/turbo-rails"
window.Turbo = Turbo
mrujs.start()

Repo (if possible)

environment

rails 6.1.4
vite ruby
Test on Chrome Android 78 which is no native support SubmitEvent API

I believe this issue is caused by repeated execution of the polyfill twice.
Object.defineProperty will set writable: false, enumerable: false, configurable: false by default.

Long story short, successful form submissions requires the form to redirect the user. Im not sure this is technically necessary.

Happy to hear other peoples feelings on this matter. The code at this point is mostly a hold-over from Turbo.

Turbo code

https://github.com/hotwired/turbo/blob/537093ba1b037b49916c4ff04b50bdd30611f6af/src/core/drive/form_submission.ts#L140-L142

Mrujs code

https://github.com/ParamagicDev/mrujs/blob/8441e6d40bfaf8b594020be6c507a5279bd7b65a/src/navigationAdapter.ts#L72-L75

Any discussion around this would be super helpful if its better to just morph anything that doesnt redirect instead of just >400 status codes. This would also mean you dont need to set a status for failed form submissions (although you probably should)

Describe Problem

the current data-ujs-confirm in the plugins section is hacky and gross. @seanpdoyle made an excellent example that shows a far more robust way to handle confirms.

Additional Details

hotwired/turbo-rails#257 (comment)

Describe Problem

Rails-ujs exposes event.detail.status and so should we!

We should also take a deeper look at exposed event.detail from rails-ujs herE:

https://guides.rubyonrails.org/working_with_javascript_in_rails.html#rails-ujs-event-handlers

According to https://m.patrikonrails.com/a-definitive-guide-to-railss-unobtrusive-javascript-adapter-ef13bd047fff

...when a developer uses redirect_to in a controller action, the client receives Turbolinks commands in raw JS:

# app/controllers/users_controller.rb
class UserController < ApplicationController
  def destroy
    User.find(params[:id]).destroy
    redirect_to users_path
  end
end

Turbolinks.clearCache()
Turbolinks.visit("http://web.dev/users",{"action":"replace"})

This may or may not be modified by the inclusion of turbolinks-rails in the project:

https://github.com/turbolinks/turbolinks-rails

It seems like this project might need to include a sidecar gem called mrujs-rails that duplicates the functionality of the above, except by sending JSON to the client. The real security issue was always generating the raw JS on the server.

Update:

It all comes down to this Concern:

https://github.com/turbolinks/turbolinks-rails/blob/master/lib/turbolinks/redirection.rb

Following the thought progression, here... it seems reasonable to assume that this is where https://github.com/jorgemanrubia/turbolinks_render comes into the picture via https://medium.com/@jmanrubia/escaping-the-spa-rabbit-hole-with-turbolinks-903f942bf52c

Expected Behaviour

I should be able to add a data-remote link to the DOM and have it work the same as one that was present at page load.

Actual Behaviour

data-remote links added to the DOM after page load don't submit a fetch request and act like normal links.

Interestingly, remote forms that are added to the DOM after page load still seem to work.

Steps to Reproduce

See https://github.com/existentialmutt/cable_car_messages/tree/mrujs . Start up the app and click on New Message, then "Cancel" .

tested with mrujs-0.3.0-beta.11

Describe Problem

currently you need to add data-turbo="false" to remote links and forms.

Suggest Solution

This DX could be improve but automatically attaching this behavior.

Details

Credit goes to @julianrubisch for suggesting the idea.

Hey Konnor, I've been so pre-occupied with cable_car I realized mrujs is doing some cool things with remote html forms and I wanted to get clear on exactly how they work. While tinkering around with a simple scaffold I noticed different behavior depending on whether I have turbo or turbolinks enabled and that seems off.

Expected Behaviour

Turbo and Turbolinks should have consistent behavior for remote form submissions.

Actual Behaviour

When using Turbolinks: browser follows the redirect with a non-turbolinks (i.e. whole page load) navigation.

When using Turbo: nothing happens

Steps to Reproduce

Start up the app in the repo provided and go to http://localhost:3000 . Fill out both fields in the form and hit submit.

Switch between using Turbo and Turbolinks by editing app/javascript/application.js

Repo (if possible)

https://github.com/existentialmutt/mrujs_sandbox

Other notes

I should add that unsuccessful form submissions work in both cases. The page is updated in-place with the HTML from the response. That's really cool!

I'd really love to know what the intended behavior should be. Coming from OG UJS where everything needs to be handled manually I'm not quite sure what to expect.

Expected Behaviour

Should show confirm dialog after clicked destroy button

Actual Behaviour

It shows an error on console
there was an error with mrujs.confirm

Steps to Reproduce

• create new blogs
• try to delete one of theme

Describe Problem

Currently, buttons require specific data-method and data-url 's. @seanpdoyle pointed out that there is an opportunity here to build upon browser fundamentals. Theres no reason a <button> cant submit an ajax request if it provides a formmethod and formaction.

Mrujs uses actual fetch under the hood, so it does not need to worry about the CSRF token generation Sean referred to. More info can be found here:

hotwired/turbo-rails#257 (comment)

Describe Problem

Rails 6.1 no longer does remote forms by default.

Suggest Solution

config.action_view.form_with_generates_remote_forms = true

Additional Details

No more lies!

Describe Problem

Maybe I just need to get better with fetch, but after catching an ajax:success event I fumbled with jumping through async hoops to get to the body content. Finally settled on event.detail.response.json().then(operations => CableReady.perform(operations)) which I guess isn't that awful but took me a few tries and some digging through fetch docs to get there.

Suggest Solution

I'd love it if event.detail provided a responseBody property (or function) that provides direct access to the response body content as a string.

Describe Problem

currently only basic elements work. We should provide a migration for custom elements and allow for users to define custom element query selectors.

Suggest Solution

Work would start here on the various selectors:

https://github.com/ParamagicDev/mrujs/blob/ca3820a7d6e62d1e34a77d62cfd3a764872c8fc4/src/utils/dom.ts#L29

Expected Behavior

204 no contents should request the current page

Actual Behaviour

204s do nothing.

Proposed Fix

Use morphdom to request the current page and then morph the response

Currently, there is a whats essentially a monkey path for fetch due to how Rails send redirects as 302 Headers. Its possible in the future we may not want to mask every method under a POST. We should allow this to be configurable.

work would be done in 2 places:

https://github.com/ParamagicDev/mrujs/blob/main/src/linkSubmission.ts

https://github.com/ParamagicDev/mrujs/blob/main/src/formSubmission.ts

These are form and link submission and masks requests to deal with how rails redirects.

I'm using some @rails/ujs methods that might be missing or need docs for compatibility to switch over to mrujs:

• Rails.fire
• Rails.confirm - I see this in my code base but I don't think its used

The following come from https://github.com/kirillplatonov/shopify-hotwire-sample/blob/910a2216dd61a5e52628e155901a42d185d1cf92/app/javascript/packs/application.js#L11-L17

• Rails.delegate
• Rails.linkDisableSelector -- oops, found it https://github.com/ParamagicDev/mrujs/search?q=linkDisableSelector
• Rails.buttonDisableSelector
• Rails.formSubmitSelector

My code base also uses Rails.ajax but AFAIK I need to port that to fetch right?

Expected Behaviour

Clicking on
<a class="text-danger" data-method="delete" data-confirm="Are you sure you want to delete this contact?" data-turbo="false" href="/sellers/2/contacts/10">Delete</a>
is expected to make a delete request to the server, where the controller action performs a redirect, and the page respond with a redirect

Controller

  def destroy
    @contact.destroy

    flash[:success] = 'Contact Deleted'
    redirect_to seller_contacts_path(current_organization)
  end

Actual Behaviour

The controller action is hit and the browser receives the response to redirect, however the DOM remains unchanged

Steps to Reproduce

1. Create an anchor with a delete method, confirm text and data-turbo="false"
2. Create a controller destryo action that responds with a redirect

Repo (if possible)

Expected Behaviour

Actual Behaviour

Steps to Reproduce

Repo (if possible)

Response vs network errors should still be separate. But they should also have a unified interface.

Currently, navigation with Turbolinks causes two responses HTMLs to fire and clobbers any flashes set by the browser.

Lines in questions:

Picture of the issues:

Describe Problem

When calling window.mrujs.fetch() should it trigger the ajax:<name> events?

Additional Details

UJS did not do this, but I view Mrujs as a superset and wonder if some cool interactions could happen from this....

Edit:

thumbsup for "yes trigger events"

thumbsdown for "no, async / await is plenty."

Describe Problem

Users cannot easily prefetch links

Suggest Solution

Expose the navigationAdapter's prefetch functionality so an end user can take advantage.

Expected Behaviour

When I destroy something with an data-method="delete", the destroy URL should not be available with the back button.

Actual Behaviour

When I destroy something with data-method="delete, the destroy url is added to the back navigation.

@seanpdoyle described a number of exciting, and intriguing points here in this PR:

hotwired/turbo-rails#257 (comment)

For example, his data-confirm implementation excites me since I currently have a very hacky data-async-confirm solution. In addition, there are many other items such as respecting frames on form submissions that is important for working in a Turbo enabled app.

There are some things that may not apply. For example, data-method in mrujs constructs a Fetch request under the hood and does not actually submit a full form, as well as some turbo-specific things such as request-start since mrujs will handle its own lifecycle of ajax: events this may not be necessary.

Regardless, over the next few days I plan to go in and pluck key points out from his post / PR and hope to start implementing some of these features into Mrujs since theyre exciting to me and I think they should be considered.

https://github.com/ParamagicDev/mrujs/blob/401b32d7cee74f8aa0463d85ee6837834768f970/src/elementDisabler.ts#L41

https://github.com/ParamagicDev/mrujs/blob/401b32d7cee74f8aa0463d85ee6837834768f970/src/elementEnabler.ts#L43

are Turbo specific and should move into the Turbo plugin.

Describe Problem

currently Turbo + Mrujs have quite a bit of intersection. I'm not even sure if this is a good idea tbh. This is more so a "future possibility"

there will be a lot of trial and error to make this happen. So this issue will serve as a future reference of possible changes that need to be made.

Suggest Solution

first change: modify querySelectors.

rails/rails@8de181d

hotwired/turbo#405 (comment)

Currently, fetch requests cant be aborted. We should add in an abort controller for each fetch request to allow it to be cancelable.

https://developer.mozilla.org/en-US/docs/Web/API/AbortController/abort

Expected Behaviour

For type declarations to be emitted to dist/.

Actual Behaviour

Only transpiled source code is emitted.

Steps to Reproduce

Run yarn build in the project root.

Repo (if possible)

https://github.com/ohmree/mrujs

I think this issue has to do with this section of the typescript plugin's readme.
In my fork (linked above) I've tried to fix it and managed to get rollup to output declarations correctly for the root project, but for plugins/ the nesting is still wrong.

One thing that did eliminate the nesting for the plugins dir was to mess with rootDir in both the plugins project and the root one (I think, can't remember exactly what I did but it shouldn't be hard to reproduce).
That makes the build fail because /plugins/ references /src/types.ts but the declarations are emitted correctly.

Describe Problem

Mrujs doesnt do things like respect frames. @seanpdoyle created a PR with what Turbo glue code would look like. I think theres a serious opportunity here to embed this into Mrujs.

https://github.com/seanpdoyle/turbo-rails/blob/0fdae4cd54e50516d94bea9ab5ea78bb911052e1/app/javascript/turbo/ujs.js