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

🐛 safer getBundles #21

Merged
merged 1 commit into from
Jun 11, 2020
Merged

Conversation

slorber
Copy link
Contributor

@slorber slorber commented Jun 8, 2020

Summary

Docusaurus v2: this plugin throws.

We write a manifest simply with:

        new ReactLoadableSSRAddon({
          filename: clientManifestPath,
        }),

Then we read it with getBundles(manifest, modulesToBeLoaded);

This leads to errors like:

image

Why

Fix error reported by Docusaurus 2 user here: facebook/docusaurus#2898

There is a Github repro, the manifest gets written to .docusaurus/client-manifest.json if you want to take a look.

I've uploaded the manifest here: https://gist.github.com/slorber/f1ba0d0fd4b7e22f0464a9782a9f9395

The asset 14 seems to be missing, producing the Object.keys(undefined) error

Checklist

At this point I'd like to know if this fix seems legit to you. For the Docusaurus 2 usecase it seems to work, as I was able to build the site.

Maybe this manifest issue hides a deeper problem. In such case we should rather keep a fail-fast behavior and fix the underlying issue instead?

Or maybe it's safe to ignore a missing asset in the manifest, I have no idea 🤪

  • 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

lex111 commented Jun 9, 2020

@themgoncalves please take a look at this.

@themgoncalves themgoncalves changed the title :bug safer getBundles 🐛 safer getBundles Jun 11, 2020
@themgoncalves themgoncalves added the bug Something isn't working label Jun 11, 2020
@themgoncalves
Copy link
Owner

@slorber thanks for your contribution! This is a really valid fix for this project.

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.

LGTM

@themgoncalves themgoncalves merged commit bd1640f into themgoncalves:master Jun 11, 2020
@themgoncalves
Copy link
Owner

@slorber @lex111 v0.2.3 was just released.

@slorber
Copy link
Contributor Author

slorber commented Jun 11, 2020

Great @themgoncalves thanks !

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

Successfully merging this pull request may close these issues.

3 participants