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

Expose mount/unmount and allow them to be scoped by a selector. #91

Closed
wants to merge 2 commits into from

Conversation

mschulkind
Copy link

Looking for comments on this before I attempt to add tests.

I'm currently using this for an app which has only a few react components, including some that are not present on page load and may disappear later, so I need to mount/unmount components from parts of the page manually in other javascript.

@@ -47,19 +51,26 @@
document.addEventListener(eventName, callback);
}
}
handleEvent('page:change', mountReactComponents);
handleEvent('page:receive', unmountReactComponents);
handleEvent('page:change', function() {mountReactComponents()});
Copy link
Member

Choose a reason for hiding this comment

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

Why are you doing this (here and below)? mountReactComponents is already a function…

Copy link
Author

Choose a reason for hiding this comment

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

It is a function, but it's not a proper event handler. If I don't wrap it like this, the event gets passed in as the rootNode.

Copy link
Member

Choose a reason for hiding this comment

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

Oh yea… duh :)

@zpao
Copy link
Member

zpao commented Sep 15, 2014

Yea, I think this is fine. This file is getting a bit longer than I wanted and I think we shouldn't export those names and should find something better before we're stuck with them being the API. At the very least we should remove React from them, and perhaps Component as well (see facebook/react#2002). I'm open to suggestions.

@rmosolgo
Copy link
Member

To me it seems outside the scope of Rails-y UJS. Historically, Rails UJS just gives behaviors based on options in your template (eg remote: true, method: :delete, data: {confirm: "..."}). I'm not aware of other UJS APIs that you call in javascript (except maybe Turbolinks.visit, if that counts 😛 ).

In my opinion it's not a good fit, especially considering that this is what React.renderComponent and React.unmountComponentAtNode are for. However if others would use this, maybe I'm wrong!

@mschulkind
Copy link
Author

Does any other UJS stuff execute on page load? I think that's part of the
problem here.

On Mon, Sep 15, 2014 at 7:19 PM, Robert Mosolgo notifications@github.com
wrote:

To me it seems outside the scope of Rails-y UJS. Historically, Rails UJS
just gives behaviors based on options in your template (eg remote: true, method:
:delete, data: {confirm: "..."}). I'm not aware of other UJS APIs that
you call in javascript (except maybe Turbolinks.visit, if that counts [image:
😛] ).

In my opinion it's not a good fit, especially considering that this is
what React.renderComponent and React.unmountComponentAtNode are for.
However if others would use this, maybe I'm wrong!


Reply to this email directly or view it on GitHub
#91 (comment).

@zpao
Copy link
Member

zpao commented Sep 15, 2014

@rmosolgo I guess the use case isn't 100% clear. I understood it that there were server-rendered things added to the page (I guess via something that isn't turbolinks) and he basically needs to rescan for things added. In that case, piggybacking on the existing infra seems fine BUT I'm not in the Rails world and don't know where people are drawing the UJS line.

But if this is meant to cover what you would normally do with React.renderComponent from your own JS that isn't server-rendered then I would probably agree that this doesn't fit.

@mschulkind
Copy link
Author

Yes, I'm dealing with server-loaded, rendered with the view helper, just
not available at DOM ready.

On Mon, Sep 15, 2014 at 7:24 PM, Paul O’Shannessy notifications@github.com
wrote:

@rmosolgo https://github.com/rmosolgo I guess the use case isn't 100%
clear. I understood it that there were server-rendered things added to the
page (I guess via something that isn't turbolinks) and he basically needs
to rescan for things added. In that case, piggybacking on the existing
infra seems fine BUT I'm not in the Rails world and don't know where people
are drawing the UJS line.

But if this is meant to cover what you would normally do with
React.renderComponent from your own JS that isn't server-rendered then I
would probably agree that this doesn't fit.


Reply to this email directly or view it on GitHub
#91 (comment).

@rmosolgo
Copy link
Member

Yeah, I don't quite understand the use case either. @mschulkind, how are you rendering with the view helper, but it isn't there at DOM ready? Does that mean the HTML is fetched by AJAX, then inserted into the document?

@mschulkind
Copy link
Author

Not 100% what I'm doing, but functionally equivalent here.

On Mon, Sep 15, 2014 at 7:31 PM, Robert Mosolgo notifications@github.com
wrote:

Yeah, I don't quite understand the use case either. @mschulkind
https://github.com/mschulkind, how are you rendering with the view
helper, but it isn't there at DOM ready? Does that mean the HTML is fetched
by AJAX, then inserted into the document?


Reply to this email directly or view it on GitHub
#91 (comment).

@rmosolgo
Copy link
Member

6 hours in and I almost understand the issue at hand, thanks for bearing with me!

I realize I actually have a similar issue. Sometimes I open a modal (inserting HTML from ajax, fancy that). To set up components, I just fake the turbolinks page:change event:

$(document).on "modal:load", ->
  $(document).trigger("page:load")

It "re-renders" existing components, but I think it's no problem. I read in the docs that calling React.renderComponent with the same descriptor will just update the component on that node (if necessary).

But now that I think about it, I don't think those components are unmounted properly. I should be unmounting those components when the modal is hidden. No biggie, I think I should just:

$(document).on "modal:hide", ->
  componentsInModal = $("#modal [data-react-class]")
  componentsInModal.each -> React.unmountComponentAtNode(this)

I guess I've gotten away with it so far since none of those components set up change listeners.

Now that I understand what's going on, I could go either way whether React UJS exposes its mounting/unmounting functions! I noticed that rails/jquery-ujs functions are available, they're just not talked about much: https://github.com/rails/jquery-ujs/blob/master/src/rails.js#L23

@mschulkind
Copy link
Author

Thanks for considering and looking into this.

While that may work, isn't that adding an awful lot of dependencies to your code? You're now dependent on turbolinks event names, the way react-ujs hooks into turbolinks, and the attribute name that react-ujs uses to mark react components. You also rely on javascript performing correctly when 'page:load' is triggered multiple times, which doesn't seem terribly safe.

@rmosolgo rmosolgo mentioned this pull request Dec 18, 2014
3 tasks
@rmosolgo rmosolgo added the 1.0 label Jan 9, 2015
@rmosolgo
Copy link
Member

@mschulkind It'd be great to get this in for 1.0, are you interested in rebasing & updating etc?

@rmosolgo
Copy link
Member

Thanks for the inspiration, this feature was implemented in #221 !

@rmosolgo rmosolgo closed this Mar 31, 2015
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.

3 participants