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

feat(ChunkExtractor): support publicPath override #292

Merged

Conversation

BRKalow
Copy link
Contributor

@BRKalow BRKalow commented Apr 1, 2019

Summary

Add support for passing publicPath to ChunkExtractor. This will allow consumers to override publicPath at runtime.

Currently in our usage we are loading the stats file, manually overriding the publicPath, and then injecting that stats object into ChunkExtractor. This seems like a simple enough change that it warrants being supported out-of-the-box.

Formatted docs section: https://deploy-preview-292--loadable.netlify.com/docs/server-side-rendering/#override-statspublicpath-at-runtime

closes #269

Test plan

Added new test cases to packages/server/src/ChunkExtractor.test.js.

@ryanboswell
Copy link

This looks great, exactly what we were hoping for 👍

@gregberge
Copy link
Owner

@BRKalow thank you for this PR! Could you please rebase and I will merge it.

@BRKalow BRKalow force-pushed the feat/chunk-extractor-public-path branch from 91e0136 to 259bd6e Compare April 3, 2019 19:45
@BRKalow
Copy link
Contributor Author

BRKalow commented Apr 3, 2019

@neoziro Done, thanks!

@lauterry
Copy link

lauterry commented Apr 5, 2019

Hi

I think this is what I was looking for.

My use case is that I call chunkExtractor.getCssString on a server at runtime to gather the critical CSS and inject it in the html before sending it to the browser.

However, now, the output is incorrect therefore when getCssString tries to look for a css file, it didn't find it. Indeed, The outputPath on build time is not the same on runtime.

@neoziro Can you confirm that chunkExtractor.getCssString relies on stats.outputPath to find the css ?

If so, please publish a new release for the new feature

Thank a lot

@BRKalow
Copy link
Contributor Author

BRKalow commented Apr 5, 2019

@lauterry I believe ChunkExtractor already supports passing in outputPath to solve the issue you are having. It looks like ChunkExtractor.getCssString does rely on stats.outputPath to get the filepath:

See here: https://github.com/smooth-code/loadable-components/blob/master/packages/server/src/ChunkExtractor.js#L186

@lauterry
Copy link

lauterry commented Apr 5, 2019

Thx @BRKalow

You're right. I'll give it a try.

Sorry for the noise. I have confounded publicPath and outputPath

@gregberge gregberge merged commit 9731e9c into gregberge:master Apr 5, 2019
@BRKalow BRKalow deleted the feat/chunk-extractor-public-path branch April 5, 2019 19:41
@lauterry
Copy link

lauterry commented Apr 5, 2019

@BRKalow

Indeed, your proposition fixed my issue. Thx

@sourabhkheterpal
Copy link

@BRKalow Hello, When you guys plan to release this to NPM ? It still gives us older version. Kindly update NPM if possible. Thanks.

@BRKalow
Copy link
Contributor Author

BRKalow commented Apr 17, 2019

@sourabhkheterpal I believe this is included in @loadable/server@5.8.0 👍

@gregberge
Copy link
Owner

Yes it is!

@srigar
Copy link

srigar commented Mar 21, 2020

@BRKalow This is awesome feature what i'm looking for currently. Added public path in chunk extractor but still js(chunk files) not loading from the runtime path which provided in publicPath.
Did I missed anything here?

extractor = new ChunkExtractor({
    statsFile: path.resolve(process.cwd(), './build/loadable-stats.json'),
    publicPath: process.env.CHUNK_ASSET_PATH
});

<ChunkExtractorManager extractor={extractor}>
        {component}
 </ChunkExtractorManager>

@natterstefan
Copy link

Hi @srigar,

I have the same issue, did you find a solution yet?

Thanks for your help.

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

Successfully merging this pull request may close these issues.

Support "on the fly" public path definitions from webpack
7 participants