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

Adds test case for desired behavior of babel plugin for user-space HOCs #43

Closed
wants to merge 1 commit into from

Conversation

lourd
Copy link
Contributor

@lourd lourd commented May 29, 2017

I discovered that when defining and using my own higher-order component in order to have a default LoadingComponent, the babel plugin does not work. It fails with a TypeError, Cannot read property 'name' of undefined.

This PR adds a failing test case demonstrating the behavior, and a snapshot of what I would expect the outcome to be. I understand the changes to the plugin necessary to produce that outcome would be pretty significant. If it's not possible, a statement should be added to the README about it not being a supported use case. You wouldn't think so right now, with the documentation of how to not repeat coming right after a recommendation to use the Babel plugin.

@jamiebuilds
Copy link
Owner

The Babel plugin came later, there's no clear way on how to make it work with HOCs. My only idea is to do something like this:

import Loadable, {loader} from 'react-loadable';
import LoadingComponent from './LoadingComponent';

function MyLoadable(opts) {
  return Loadable({
    LoadingComponent,
    ...opts,
  });
}

let MyLoadableComponent = MyLoadable({
  [loader]: () => import('./MyComponent'),
});

Where loader is a well-known symbol and the Babel plugin could work off of it.

@lourd
Copy link
Contributor Author

lourd commented May 30, 2017

Not a bad idea. I could jibe with that API. If I write it, will you merge it?

@faceyspacey
Copy link

@lourd
Copy link
Contributor Author

lourd commented Jun 8, 2017

Nice writeup! You've been crushing it 👊🏼

Next.js style 2 argument API

I don't understand what you're referring to

@faceyspacey
Copy link

Their dynamic function async takes an async component and options whereas this just takes options:

dynamic(asyncComponent, options).

So the first arg is always what was assigned to the loader key in React Loadable.

...on a side note, u have seen webpack's new magic comment feature right? That along with dynamic require paths allow U to define one hoc that dynamically chooses between multiple chunks/modules idiomatically and with one function. That will crush it for the HoC thing. There's actually a related issue I submitted to webpack or I would have wrote about it. resolveWeak doesn't yet fully work with dynamic module paths. Once we have his capability, the hoc stuff ur after will become even more important. In fact if ur like me, if ur code splitting, it almost always is done where u dynamically choose between a bunch of modules in a given category to get the most bang for your buck. Fortunately sokra marked it with high priority so I have a good feeling this will get done soon. They too have invested a lot in code splitting and likely get the importance of the path React Loadable pioneered.

Here's what needs to be fixed:
webpack/webpack#4993

@jamiebuilds
Copy link
Owner

The new Babel plugin handles this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants