Loadable-components: Failing to import named exports server side

[robdonn]

🐛 Bug Report

When importing a named export from another file, I receive the following error

{
  "status": "error",
  "message": "Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: object.",
  "stack": [
    "Invariant Violation: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: object.",
    "at invariant (webpack:///../node_modules/react-dom/cjs/react-dom-server.node.development.js?:58:15)",
    "at ReactDOMServerRenderer.render (webpack:///../node_modules/react-dom/cjs/react-dom-server.node.development.js?:3395:7)",
    "at ReactDOMServerRenderer.read (webpack:///../node_modules/react-dom/cjs/react-dom-server.node.development.js?:3131:29)",
    "at renderToString (webpack:///../node_modules/react-dom/cjs/react-dom-server.node.development.js?:3598:27)",
    "at eval (webpack:///./src/server/render.jsx?:40:90)","at Layer.handle [as handle_request] (webpack:///../node_modules/express/lib/router/layer.js?:95:5)",
    "at trim_prefix (webpack:///../node_modules/express/lib/router/index.js?:317:13)","at eval (webpack:///../node_modules/express/lib/router/index.js?:284:7)",
    "at Function.process_params (webpack:///../node_modules/express/lib/router/index.js?:335:12)","at next (webpack:///../node_modules/express/lib/router/index.js?:275:10)"
  ]
}

To Reproduce

Express SSR setup

App.js

import React from 'react'
import loadable from '@loadable/component'
import './main.css'

const A = loadable(async () => {
  const { A: AComponent } = await import('./letters/A')
  return () => <AComponent />
})

const App = () => (
  <div>
    <A />
  </div>
)

export default App

./letters/A.js

export const A = () => 'A'

Expected behavior

Named export should resolve server side and render correctly

Link to repl or repo (highly encouraged)

Example Repo

[gregberge]

Hello @robdonn, currently "custom async function" is not supported server-side. The only solution for now if to make a "simple import statement" and to not use named imports. It could work in future, but currently it is not the priority.

[frosas]

Should this be mentioned in https://www.smooth-code.com/open-source/loadable-components/docs/babel-plugin/ perhaps?

[theKashey]

Like "only direct imports are supported", and "use render method to render a named export", like it was documented in v3, however it's not exported in the current version.

[frosas]

@theKashey I'm referring to the same constraint that also applies for React.lazy: https://reactjs.org/docs/code-splitting.html#named-exports

[theKashey]

Me too, as long as to use named exports you have to wrap your import with some logic, and you have to use a _"direct"/"bare"/"naked"_ one.

[frosas]

Ah, sorry, I wasn't so sure we were talking about the same.

So, would it possible to have this constraint documented? I'm pretty sure new users will thank it.

[theKashey]

I reckon it should be absolutely clear, as long as I've was reported a few production incidents due to this behaviour.
Even more - it should have an automated test, checking result of the import with the module cache.

[hedgepigdaniel]

This seems like quite a large problem to me:

• behavior is different on client/server in a way which is completely surprising when looking at app code
• the resulting bug is very bad - crash/missing component
• the docs actually recommend that you use an async wrapper around the import to implement delays and timeouts - so its natural to expect that this will also work server side.

Am I right to think that this is difficult to implement on account of the server require needing to be synchronous, and calling the user's async resolve function would break that?

Perhaps the babel plugin could more easily be patched to throw an error in the case that loadable is called on something which is not a simple import() (perhaps with exceptions for specific things which only need to work on the client, e.g. timeout). Then there could be a separate API for the user to provide a synchronous function (<module>) => <component> to replace the default.

[theKashey]

In short, there are two ways how to fix this problem, both are more or less fundamental:

• not allow a user to use any "extra" code in the imports. Remove the possible ways to cause the bug. This is about validations, invariants and other safety nets.
• allow that extra code to be used. This is about mixing synchronous and asynchronous behaviours, and with loadable ability to handle "full dynamic imports"(read props) - that's a bit complicated. From another point of view the problem could be solved by __removing__ server-side specific code, but that would require or some API change, or, well __Suspense__ on SSR.

Way 1 is what the majority of us need. You might import only default export and deal with it... However, WFT per minute would be quite hight.
Way 2 is how it should work, but it requires some features, which are not yet here, but should be more or less soon.

Shall be just wait a bit?

[hedgepigdaniel]

Sure, it seems like supporting true asynchronous wrappers is difficult until React supports suspense in SSR.

That said, it strikes me that the use case of choosing an export in the resolved module is not fundamentally asynchronous. What about the following API (perhaps not ideal, but doable without SSR suspense). The basic idea is to remove anything asynchronous from the API as much as possible, and separate

• the API (synchronous) for providing the imported module path: path
• the API (synchronous) for resolving the component from the imported module: resolveComponent
• asynchronous extra behavior which only works on the client side: watchdog

type Loadable<Module, Props> = (options: {
  /**
   * Path of the the module containing the component, or a function
   * which returns the path given the props
   *
   * NOTE: The babel plugin will create the import()
   */
  path: string | ((props: Props) => string);

  /**
   * Watchdog is able to implement delays, timeouts etc on the client side only.
   * Receives the import promise and returns the watchdog promise.
   * The resulting loadable component does not render until the watchdog promise
   * resolves, and considers the import to have failed if the watchdog promise rejects
   *
   * Default: `() => Promise.resolve()`
   */
  watchdog: (importPromise: Promise<Module>) => Promise<void>;

  /**
   * Synchronous function that returns the component from the
   * imported module.
   *
   * Default. `({ default: Component }) => Component`
   */
  resolveComponent: (module: Module, props: Props) => React.Component<Props>;
}) => LoadableComponent<React.Component<Props>>;

Then you could do something like

const LoadableA = loadable({
  path: './A',
  resolveComponent: ({ A: component }) => component,
  watchdog: (importPromise) =>  pMinDelay(importPromise, 200),
});

resolveComponent here is quite similar to the resolve option for the existing createLoadable - but createLoadable is not exposed to the user and resolve is also responsible for other stuff (hoisting)

Benefits:

• No opportunity for the user to try wrapping the import() in anything that changes the resolved value - they just provide a path. Transforming a path to an import should be easy in the babel plugin.
  
  
  
  • Side note: Hopefully this can be typed easily... another option is to have createImport and the user profides an import function, but the babel plugin can enforce (as well as possible) that it really is just an import function. Probably this would also allow it to work without the babel plugin.
  
  
• User is free to use named exports, and any synchronous logic to wrap the imported component, and this logic works on client and server
• User is free to use any asynchronous logic on the client to delay/cancel loading
• No need for SSR suspense

[theKashey]

• string based names are 😭- no autocomplete, no linting, no types. Only require or import might bring types to the system, so they have to be used.
• why you need names imports in a first place? You want to add a mapper(resolveComponent) here, but why not to create another file, import what you want there, and export "that" as a default export? Follow the "standard interface", and define a clear boundary.
• why you might want to delay import? If it's needed - then what else might be needed? Could it be a part of resolveComponent(it might be an async function)

[hedgepigdaniel]

• fair enough - like I said having a createImport instead also works, but the babel plugin can enforce that createImport is only () => import(<path>), not () => someWrapperWhichFailsServerSide(import(<path>)) (This seems harder for the babel plugin, but easier for code analysis tools)
• I don't like default exports. They encourage the same thing to have different names in different files - so grepping for the use of an export is difficult. If a named export is called XYZ, all files that import it contain the string XYZ. Not true with default exports. Also IDE autocomplete for imports is unlikely to work well with default exports, but it works great for named exports in VSCode. Named exports is the only reason I would want to use resolveComponent, but I think it's a good reason. resolveComponent could be replaced with a key (the exported name with the component), but I think this would be harder to type.
• That's how it currently works - but I think the API is confusing. It's confusing because currently you can wrap it with some async function, e.g. the way the docs suggest for delay/timeout. The confusing part is that it looks like you can make that wrapper return a default value (as in OPs example) but in fact that fails on SSR and suceeds on the client. Like you said, this can easily lead to bugs. So I separated out watchdog from the import promise because that makes it clear that the user is not allowed change the return value of the import promise, and allows the babel plugin to enforce that without breaking the ability to have timeouts and delays.

type Loadable<Module, Props> = (options: {
  /**
   * Function receiving the props which returns a promise resolving to
   * the module that contains the inner component.
   *
   * NOTE: Wrapping the `import()` is not allowed and will cause an error
   * in the babel plugin! It must be literally `(props) => import(<something>)`
   */
  createImport: (props: Props) => Promise<Module>;

  /**
   * Watchdog is able to implement delays, timeouts etc on the client side only.
   * Receives the import promise and returns the watchdog promise.
   * The resulting loadable component does not render until the watchdog promise
   * resolves, and considers the import to have failed if the watchdog promise rejects
   *
   * Default: `() => Promise.resolve()`
   */
  watchdog: (importPromise: Promise<Module>) => Promise<void>;

  /**
   * Synchronous function that returns the component from the
   * imported module.
   *
   * Default. `({ default: Component }) => Component`
   */
  resolveComponent: (module: Module, props: Props) => React.Component<Props>;
}) => LoadableComponent<React.Component<Props>>;

[theKashey]

I don't like default exports. They encourage the same thing to have different names in different files

Well, not in __all__ files. Think about this like about API interface - a function accepts some shape, and you have to provide that shape. And there is a _share_ which is quite natural and easy to use - a default export.

For example - we do use Feature/index.tsx+Feature/Component.tsx, where index.tsx is everything we want to "export" for Feature, and the rest is an _internal implementation_.
For the lazy components, we use Feature/split.tsx, without any index - that a part of our file naming convention, and a part of our API.

import('Feature/split') sounds absolutely correct, and not causing any issues with "how to import that thing". That thing is a Feature, and there is only one to import it. However, manually importing "that thing" is also incorrect, so we do have Feature/lazy (still no index) where we do import "that thing" as we want, and re-export lazy-ed component as a __named export__, so autoimport feature work correctly.

Just a simple example how this problem could be solved from a different point of view.

[hedgepigdaniel]

That sounds like a good convention, and sure, if everyone 100% follows it always it works. The way I like to do it is to always use named exports and never use default exports. If you see a named export, you can 100% grep for it and it will work, you can be sure of it. With your convention its still possible that someone directly imported one of the default exports in the internal implementation (maybe they shouldn't, but they did) - and then if you change it something breaks. Even if that's rare, if its not 100% you can't trust it and refactoring is slower.

I take your point - its not necessary for loadable components to support named exports, but I think it is useful and important. I like to use an eslint rule enforcing named exports - so to use loadable-components I have to make the eslint config more complicated to add exceptions for loadable-components, or drop the rule (and then probably have default exports everywhere again).

The "large problem" IMO is the surprising different behaviour on SSR vs client if you try to wrap the promise (which can very directly cause bugs). To solve that, the best idea I can think of is to do a separation in the API like createImport vs watchdog, and then make the babel plugin detect if the return value is not a direct import() statement and error otherwise. I think that would be simple to implement, and it would solve this issue. Adding resolveComponent would mean not breaking support for named exports for people who don't use SSR.

[theKashey]

It's more about - what we could do to support this

• 100% we should separate "importer" and "mapper"
• 80% we will not break existing API with it, as long as ... it's fatally broken right now for this case.
• are we missing any other case before making a change?

[FKobus]

I would love to see this feature implemented! I want to group several components from one entry point without creating a massive amount of extra chunks.

For example, a profile page which is only loaded after login and that 'chunk' contains the profile subpages. The way it's setup now I get multiple chunks for each subroute under the profile namespace.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[theKashey]

Sorry Stalebot, but it's nearly done.

[1846689910]

Hi @theKashey, I wonder if there is ETA for this named export loadable component feature

[theKashey]

You might find more details on the PRs, but look like we are 🐢

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[hedgepigdaniel]

this is not fixed yet

[theKashey]

And still in high demand.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[theKashey]

Resolved in v 5.13.0 via resolveComponent option. Please refer to the documentation.