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

Full Dynamic Import failing @5.14.0 #649

Closed
Marlrus opened this issue Oct 21, 2020 · 11 comments
Closed

Full Dynamic Import failing @5.14.0 #649

Marlrus opened this issue Oct 21, 2020 · 11 comments
Assignees

Comments

@Marlrus
Copy link

Marlrus commented Oct 21, 2020

💥 Regression Report

After the update to 5.14.0 components were not loading with the previous syntax:

// package.json
"@loadable/component": "^5.12.0",

This makes our component update to the latest version that is a minor release: backward compatible new features.

Last working version

Worked up to version: 5.13.x

Stopped working in version: 5.14.0

To Reproduce

Steps to reproduce the behavior:

Pattern used: Full Dynamic Import

  • Install npm install @loadable/component
  • Use the full dynamic import by using props to import the component
  • Dynamically import a component inside another component by passing the path in as a prop

Expected behavior

This is the structure we were handling:

import loadable from '@loadable/component';
...

const DynamicComponent = loadable(props => import(`${props.component}`));

//component
const MyComponent = props => {
   ...
  <DynamicComponent
    // props.componentPath has the path to the component that is being loaded
    component={props.componentPath}
    ... 
  />
  ...
}

The expected behavior was that the component gets loaded after passing the path.

Run npx envinfo --system --binaries --npmPackages @loadable/component,@loadable/server,@loadable/webpack-plugin,@loadable/babel-plugin --markdown --clipboard

Script results:

## System:
 - OS: Linux 5.4 Ubuntu 20.04.1 LTS (Focal Fossa)
 - CPU: (8) x64 AMD Ryzen 5 3500U with Radeon Vega Mobile Gfx
 - Memory: 434.11 MB / 9.73 GB
 - Container: Yes
 - Shell: 5.0.17 - /bin/bash
## Binaries:
 - Node: 12.18.4 - ~/.nvm/versions/node/v12.18.4/bin/node
 - Yarn: 1.22.5 - /usr/bin/yarn
 - npm: 6.14.6 - ~/.nvm/versions/node/v12.18.4/bin/npm
## npmPackages:
 - @loadable/component: ^5.12.0 => 5.14.0 

Note: These are the setting on my local machine, the settings on the production server were different and, unfortunately, I don't have permissions to get the logs directly, and the package was already reverted which resulted in the expected functionality.

We reverted to 5.12.x and things worked

@open-collective-bot
Copy link

Hey @Marlrus 👋,
Thank you for opening an issue. We'll get back to you as soon as we can.
Please, consider supporting us on Open Collective. We give a special attention to issues opened by backers.
If you use Loadable at work, you can also ask your company to sponsor us ❤️.

@theKashey
Copy link
Collaborator

Sounds like #630

@Marlrus
Copy link
Author

Marlrus commented Oct 22, 2020

Hi theKaskey thanks for the quick reply. I looked at the thread for #630 and #629. I also reproduced the conditions for the unexpected functionality to go deeper into the details.

I get no errors in the terminal at all, the component just doesn't render. I get to the page where the component gets rendered dynamically and it is just a blank. If I revert to something before 5.14.0 the component displays once more.

Is there any way I can test for what you mean in #630 to see if my component is static and if I'm deriving a cache key?

Help would be greatly appreciated and I would love to dig and learn where the issue might be.

Cheers!

@theKashey
Copy link
Collaborator

No error and no component? Can you inspect it in dev tools? Is it SSR or purely client side stuff?

@Marlrus
Copy link
Author

Marlrus commented Oct 22, 2020

Indeed, no error and no component. Currently I am getting the default fallback component which is a Map Component wherever the other components should be.

It is in a CSR app using Create React App.

Using DevTools I can see in the component tree that there are two instances of InnnerLoadable The top one has the prop innerComponent with the path to the component ./components/V.2.0/SelectNumber/selectNumber the second instance has the componet prop as the path ./components/V.2.0/SelectNumber/selectNumber.

This instance is the one that is failing to import using the props.component value. Every component that renders this way is defaulting the the Map component which is on a totally different path.

@theKashey
Copy link
Collaborator

Ok. Much better!
So they are rendering something, but something completely different from the expected!

  • How different? Can you provide file paths?
  • CSR only
  • Is it really not working only in 5.14.0 or 5.13.2 is also affected?
  • Not sure... but you've said that it renders the first component, and failing to render the second?
  • Is there any example to play with? I've tried creating one, and everything working great

@KACAH
Copy link

KACAH commented Oct 29, 2020

I think I solved this problem (had it too). The reason is not using loadable babel plugin AND not specifying manual cache key for pages as written in docs.

It worked in 5.13.2 (specifically before this commit) because all Page props were serialized in JSON and used as cache key by default. Normally Page props contain component path which might be unique in project context with high probability. So JSON fallback was unique too.

Technically there is a regression in 5.14.x, but it is solved simply by reading and following docs :)

@theKashey
Copy link
Collaborator

@KACAH - so you were writing "unrolled imports" manually, and accidentally breaking contracts?
Well, I am thinking about a few more places which might not work as expected...

Was not considering this option as an option. Can you share your reasons not to use the babel plugin?

@KACAH
Copy link

KACAH commented Oct 29, 2020

Hmmm. I am not sure, what do you mean by "unrolled imports" and contracts. Can you explain, please? I was using "@loadable/component" like this:

const Page = loadable(props => import(`./${props.component}.screen`), {
  fallback: <Loading />,
})

....

for (const route of routes) {
   ....
    <Route path={route.path}>
        <Page component={route.component} />
    </Route>
}

So, basically, I have pages of my application (URLs) separated to different component files (with .screen postfix) and loaded dynamically.

Can you share your reasons not to use the babel plugin?

I am trying to avoid any additional dependency as long as it is reasonably possible. Especially when it comes to "build" dependencies. More dependencies -> More code -> Less stable the whole system is. Plus, I realy cringe to see things like this this in my yarn.lock file

camel-case@3.0.x:
  version "3.0.0"
  ...

camelcase-css@^2.0.1:
  version "2.0.1"
  ...

camelcase@^5.0.0, camelcase@^5.3.1:
  version "5.3.1"
  ...

camelcase@^6.0.0:
  version "6.2.0"

Do I really need four libraries to make string camel case...

@theKashey
Copy link
Collaborator

Ok. Got you. Look like that commit broke non-babelified integrations, and is not required for babelified ones (with ctor.resolve)

I'll double check, but probably the easiest way is to:

  • roll it back
  • add dev time warning about passing nonserializable variables to props
  • use more resilient serializer.

@theKashey theKashey self-assigned this Oct 30, 2020
@theKashey theKashey added the bug label Oct 30, 2020
@stale
Copy link

stale bot commented Dec 29, 2020

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.

@stale stale bot added the wontfix label Dec 29, 2020
@stale stale bot closed this as completed Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants