Skip to content

Conversation

@alexjreich
Copy link

Added chokidar to replace webpack watching so that the cache plugin can work. Otherwise, webpack doesnt see any file paths that should be watched if build isnt required.

@loppear would appreciate your eyes on this given its a bit of a rewrite.

…an work. Otherwise, webpack doesnt see any file paths that should be watched if build isnt required
@alexjreich alexjreich requested a review from loppear September 25, 2019 17:32
@alexjreich
Copy link
Author

@loppear can you review the latest changes? Updated to use template level caching for includeScript, so that a single dependency tree for multiple components is created. Obviously, this only works if components are using the same template name to include under - maybe we consider a single clientjs include file on production?

Copy link
Contributor

@loppear loppear left a comment

Choose a reason for hiding this comment

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

Hmm. This bundleByTemplate seems liable to reintroduce the inefficiency that #27 tried to remove of proliferating output duplicates by templateName. But, doing it by script name assumes that the application can be structured with entry point scripts that include/import the relevant dependencies for a bundle. Is there a change in how we're doing react or reusable or granular includes that makes that invalid?

@loppear
Copy link
Contributor

loppear commented Oct 14, 2019

That is, this helps with N includeScript('view.ejs', 'granularComponent1.js') calls, whereas current expects N includeScript('view1.ejs', 'bundleEntry.js') to be common, where bundleEntry.js -> import granularComponent1; import granularComponentN;. Problematic to just go back to preferencing the former, I can imagine an app modularization that makes the former preferable but would want to confirm.

If we need both approaches, I'd lean towards a separate config not call-time (this decision is definitely application-specific) for how bundles should be written, which could include by script, by template, all-in-one. Or perhaps this byTemplateName is overloading template as a standin for a new argument/name that allows the bundleEntry approach but across unaware modules.

@alexjreich
Copy link
Author

alexjreich commented Oct 14, 2019

@loppear main difference is that bundleByTemplate will take all scripts for a template and pass as a single entry point array to webpack. Same result as manually creating an entry point file that includes all the reference scripts, so no overwriting/duplication as in #27. Main benefit is that Webpack will only include a single include dependency for the entire package, even if ref'd multiple times in different script files (e.g. only one instance of React loaded, even if multiple imports in different files)

@loppear
Copy link
Contributor

loppear commented Oct 14, 2019

I'm pretty sure that's not a difference, for the case that 27/entry-point script was addressing, and that this would regress? I get this is automatically doing the consolidation that an entry point script does, for the case where template-name == bundle, but I think the benefit of that is limited to your particular structure?

@loppear
Copy link
Contributor

loppear commented Oct 14, 2019

@mjreich but if I'm confused about whether this regresses, and it actually just moves us closer to all-in-one rather than partitioning by template name, all good.

@alexjreich
Copy link
Author

@loppear yeah, maybe I don't understand what #27 did - I thought it was to address the case where two scripts both bound to 'default' as the template name; the old behavior was for only the last script to be served (any previous script on that template was overwritten)?

@loppear
Copy link
Contributor

loppear commented Oct 7, 2020

Time to revisit, this is published but still not right, probably there's a combination of #29 and the removal of the OnlyIfChanged plugin here, but chokidar isn't being used at all here and the number of scripts generated now is not ideal.

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.

3 participants