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

Extend entry support #8

Merged
merged 2 commits into from
Sep 4, 2018
Merged

Extend entry support #8

merged 2 commits into from
Sep 4, 2018

Conversation

havenchyk
Copy link
Contributor

Add a basic support of object and array for entry property

accordingly with
#3 (comment)

src/index.js Outdated
@@ -92,7 +92,10 @@ export default function PrerenderLoader (content) {
async function prerender (parentCompilation, request, options, inject, loader) {
const parentCompiler = getRootCompiler(parentCompilation.compiler);
const context = parentCompiler.options.context || process.cwd();
const entry = './' + ((options.entry && [].concat(options.entry).pop().trim()) || path.relative(context, parentCompiler.options.entry));
const entry = './' + (
(options.entry && [].concat(options.entry).pop().trim()) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can probably safely use options.entry here since it's a user-supplied option rather than a config value - if someone passes entry[]=1&entry[]=2 it should really be erroring out like it does today rather than silently ignoring the first entry haha.

Choose a reason for hiding this comment

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

I am a bit concerned about the assumptions made for an array and object entry. From webpack docs, having an array entry would make all elements as the source for a single chunk, so all the entry points should be used for the compile. For an object, while the suggestion is main as the main entry, the config is such that those keys would be the names of the resulting bundles, and there is no guarantee that the first (alphabetical) key is the entry point. Moreover, if this loader is used with multiple HtmlWebpackPlugins, a SPA for static routes for instance, then maybe the entry point should mirror the webpack chunks that go into that static page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rkichenama that makes sense, please, check the last commit. Would this idea be better than initial one?

@developit
Copy link
Collaborator

Looks good! Before we merge this, I want to double-check that there isn't already a way to handle this via Webpack (SingleEntryPlugin vs MultiEntryPlugin). Otherwise I think this will work great!

@havenchyk
Copy link
Contributor Author

havenchyk commented Jun 1, 2018

@developit From what I see, MultiEntryPlugin works the same way as Single, but we can pass an array there. There is also another like EntryPoint plugin, but I'm not sure how to use it because I'm not familiar how to apply it(constructor doesn't accept arguments and plugin relies only on event), but it handles all type of entry (array, object and string)

@havenchyk
Copy link
Contributor Author

@vaxilicaihouxian
Copy link

Does this commit will fix the bug that entry is an object ?

@havenchyk
Copy link
Contributor Author

@vaxilicaihouxian I hope so. https://github.com/GoogleChromeLabs/prerender-loader/pull/8/files#diff-c193db11b3837e38bd24c06b98cb93ffR38 this line uses entry as an object

@vaxilicaihouxian
Copy link

@havenchyk But it seems that this commit has not been merge to the master branch.How can I use it?Pull from your fork branch?

@developit
Copy link
Collaborator

Sorry for the delay! I have a project setup to test this easily now.

@developit developit merged commit 913787b into GoogleChromeLabs:master Sep 4, 2018
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.

None yet

4 participants