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

🐛 Add all chunks from stats object #20

Merged
merged 1 commit into from
May 29, 2020

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented May 18, 2020

Summary

In my project, I encountered an error (see below), in the end I found out that not all chunks are added to manifest, I tried to fix it.

Why

In my code, I ran into a problem when the chunk is used, but was not defined in the assets property, which is why I got the following error (from getBundles).

TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at Array.reduce (<anonymous>)TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at Array.reduce (<anonymous>)

As a rule, chunk.ids contains only one element - this is chunk.id, but there are cases when there may be several ids, and it seems to me that we should take this fact into account.

@themgoncalves honestly, I don’t know for sure if this solution is right, but it solved my problem. I would be grateful if you could help me improve this change.

Checklist

  • Your code builds clean without any errors or warnings
  • You are using approved terminology
  • You have added unit tests, if apply.

@lex111
Copy link
Contributor Author

lex111 commented May 28, 2020

Hi @themgoncalves, any thoughts about this PR?

@themgoncalves themgoncalves self-requested a review May 28, 2020 15:00
@themgoncalves themgoncalves added bug Something isn't working investigation This issue needs an investigation labels May 28, 2020
@themgoncalves
Copy link
Owner

Hi @lex111,

Thanks for your contribution, I will be debugging the code and making sure that we got it all covered, but for that, I need to have more infos so I can a test environment for that.

Do you mind to share a code demo that runs the issue you reported?

@lex111
Copy link
Contributor Author

lex111 commented May 28, 2020

We use your lib in Docusaurus, you probably remember this project.

Reproducible demo you can find here https://github.com/facebook/docusaurus/tree/repro-2244/website-build-fail

When you try to build that site by running the yarn build command, you get the error that I indicated in the issue.

This happens when the user overrides the NotFound component (see https://v2.docusaurus.io/docs/using-themes/#swizzling-theme-components). If you delete this file, then there will be no fatal errors when building the site.

I don’t understand why this is happening, I just found out that it is due to react-loadable-ssr-addon
So I will be very glad if you can help with this strange error. I hope you have enough information for debugging?

@themgoncalves
Copy link
Owner

Yes, this is enough information. I’ll be getting back to this topic tomorrow.

Thanks for sharing such detailed infos.

Copy link
Owner

@themgoncalves themgoncalves left a comment

Choose a reason for hiding this comment

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

Code wise and functionality looks good.
Couldn't find any possible issues that this PR could introduce, so we're good to go.

@themgoncalves themgoncalves merged commit 7f64458 into themgoncalves:master May 29, 2020
@lex111
Copy link
Contributor Author

lex111 commented May 29, 2020

@themgoncalves thank you! Although I do not understand why and how more elements can appear in chunks array. But in any case, this PR should not bring breaking changes. Could you please release a new version?

@themgoncalves
Copy link
Owner

@lex111 v0.2.1 was released: https://www.npmjs.com/package/react-loadable-ssr-addon

@themgoncalves
Copy link
Owner

@lex111 As precaution measure I just release another patch with a few packages downgrade, as it was showing flaky behaviour which concerns me.

@lex111
Copy link
Contributor Author

lex111 commented May 29, 2020

@themgoncalves I got you, so I will use v0.2.2 then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working investigation This issue needs an investigation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants