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

Access wrapped component #119

Closed
edongashi opened this issue May 18, 2018 · 16 comments
Closed

Access wrapped component #119

edongashi opened this issue May 18, 2018 · 16 comments

Comments

@edongashi
Copy link
Contributor

edongashi commented May 18, 2018

Hi, is it possible to access wrapped component such as:

const Universal = universal(() => import('MyComponent'))

// after some time, I need to do some actions based on static properties of the original component
if (Universal.wrappedComponent && Universal.wrappedComponent.doSomething) {
  // do something
}

// I cannot rely on hoisting, because the imported component may not have that static declared
if (Universal.doSomething) // I can't be sure if the static is missing or if the component has not yet loaded

I know about the callbacks, I was just wondering if there is a shorter way for this... Thanks!

@matthew-dean
Copy link

I have the similar question, but related to instance methods, not static methods. I'm wrapping react-player like this:

import universal from 'react-universal-component'
const ReactPlayer = universal(() => import(/* webpackChunkName: 'library' */ 'react-player'), {
  resolve: () => require.resolveWeak('react-player'),
  chunkName: 'library',
  loading: emptyNode
})

And I'm trying to set a reference to the player, so I can call methods, like the example here: https://github.com/CookPete/react-player/blob/master/src/demo/App.js#L101-L103

The problem is that ref attaches a reference to the ProxyComponent rather than passing / attaching a reference to react-player. The ProxyComponent doesn't seem to maintain any instance reference to the wrapped component, making the instance methods inaccessible.

How do we use the ref React property with this library?

@edongashi
Copy link
Contributor Author

There is some new API with refs, haven't look at it in depth yet, but maybe it can help you with your problem: https://reactjs.org/docs/forwarding-refs.html

@edongashi
Copy link
Contributor Author

edongashi commented May 19, 2018

Another annoyance is that hoisting happens only when a render has been initiated, I cannot access static properties before the initial render (the initial load contains the flushed module, so it's available sync).

Any chance for a preloadWeak() that returns something synchronously or hoists the component if it's loaded?

@matthew-dean
Copy link

@edongashi That helps, but react-universal-component itself would need to (and should) implement the forwardRef API to pass along refs to components it wraps. There's never any need to access the universal component itself via a ref assignment.

@ScriptedAlchemy
Copy link
Collaborator

Miss read this, but are all the dependencies up to date? Babel loaders and such?

@edongashi
Copy link
Contributor Author

Yes, this is not a bug report, more like a feature request.

Universal component does not check if the wrapped component is available sync before starting an initial render cycle. That is okay for instance side, but for static side you won't be able to access statics without starting to render and hook onAfter.

I wanted to implement something like next's getInitialProps where I get some data which will be passed as props to the component itself. If I have to mount it first then I am forced to do a double render from server side, because some static methods may have side effects or may result in route changes (new render).

To have full control I wanted to do some static analysis on the component and then render it. Doing side effects during mounting does not feel right, especially in the server where you are in no hurry to render anything except the final view.

I will see about testing the PR'd code. Matthew also requested a feature, so we should see about fixing child refs too.

@ScriptedAlchemy
Copy link
Collaborator

I’m currently working on some much desired Webpack 4 upgrades for the universal family. After that’s done, I can try and look into this, if you have the drive to lay the groundwork on the refs related stuff- we can try to collaborate on it.

There’s a lot happening with Some big upgrades that we are gonna release, time constraints are hard to manage 😋

@edongashi
Copy link
Contributor Author

Sure, I'll do whatever I can to help. Good luck on the webpack 4 stuff!

@zackkirsh
Copy link

Thanks! Last thing to knock out the park: faceyspacey/extract-css-chunks-webpack-plugin#60

@matthew-dean
Copy link

matthew-dean commented May 30, 2018

@zackljackson @zackkirsh @edongashi Yeah, basically our team is looking for universal components to pass in ref={...} references to the wrapped component, so that wrapping in universal doesn't break access to the underlying component instance:

const Avatar = universal(() => import(/* webpackChunkName: 'elements' */ 'common/presentation/elements/Avatar'), {
  chunkName: 'elements',
  loading: emptyNode
})
// ...
setRef = node => {
  // this node should be an instance of <Avatar>, NOT <UniversalComponent>
  // The universal component is not correctly forwarding refs
  this.container = node 
}
render () {
  <Avatar ref={this.setRef}>
}

IMO correctly forwarding the ref should solve both feature requests, because the instance would be accessible, and the instance would have static values (as well as instance values).

@ScriptedAlchemy
Copy link
Collaborator

If you can make a PR and a test, I’ll merge it for you. You might get it done quicker than I can right now

@matthew-dean
Copy link

@zackljackson Fair enough! I'll see how critical this is to our project. If it is, there might be time for it.

@ScriptedAlchemy
Copy link
Collaborator

@matthew-dean lets open a separate issue for ref forwarding, ive seen it around so it might be simple to do.

@matthew-dean
Copy link

@zackljackson Okay, do you need me to do that or were you going to do that?

@OliverJAsh
Copy link
Contributor

Did anyone open an issue to track ref forwarding?

@ScriptedAlchemy
Copy link
Collaborator

dont think so

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

No branches or pull requests

5 participants