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

Bundles in Shell strategy should not link to the Shell. #1944

Closed
usergenic opened this issue Apr 10, 2017 · 5 comments
Closed

Bundles in Shell strategy should not link to the Shell. #1944

usergenic opened this issue Apr 10, 2017 · 5 comments

Comments

@usergenic
Copy link
Contributor

Currently the Bundler treats a Shell as just-another-bundle which is mostly correct; the problem is that the Shell, being already loaded prior to other bundles is should not be linked to from within other bundles.

It happens whenever a bundle's shared import is bundled into the Shell and rewriting of the html import occurs:

Entrypoint/fragment before bundling:

<link rel="import" href="some/shared/dependency.html">
<link rel="import" href="other/shared/dependency.html">

Entrypoint/fragment after bundling:

<!-- after bundling -->
<link rel="import" href="shell.html">

The effect of an unnecessary import link to the Shell is generally negligible/idempotent/mostly-irrelevant from the browser's perspective, since the browser should notice the Shell is already loaded and it will skip the duplicate link.

However, in the case of generating the push-manifest in the CLI, it turned out to need a specific workaround to prevent all fragments from including preload declarations for the Shell itself.

Before fixing this explicitly as a function of a 'shell' being defined, it might be worth considering whether we can rely on the lazy-import declarations to build a directed graph which we could use to exclude/strip back-linking. I.e. if bundle A is always assumed to be loaded before bundle B then any shared dependencies B references that are part of bundle A can be stripped out of B at bundle time instead of rewritten to reference bundle A.

@web-padawan
Copy link
Contributor

web-padawan commented May 3, 2017

@usergenic what is the status of this issue? We faced a bug when using gulp-rev-all: any shell update makes its hash to change, which triggers change for all the bundles (because they all import shell). So we revalidate cache much often than needed 😞

@usergenic
Copy link
Contributor Author

@web-padawan This has been a low priority bug because downstream effects of this were considered mostly innocuous. Thanks for reporting another scenario where this does have effects.

Right now the bundle manifest doesn't provide advice for things like stripping out links, which is essentially what it would need to do.

I don't want to hardcode for this with a specific workaround just for shell-- Hopefully I can put together a design document PR for this issue soon.

@web-padawan
Copy link
Contributor

Thanks for prioritizing this. Actually I had to use gulp-replace for a workaround:

.pipe($.replace(/<link rel="import" href=".*my-app\.html">/g, ''))

@usergenic usergenic self-assigned this Jun 26, 2017
@usergenic
Copy link
Contributor Author

usergenic commented Jun 28, 2017

This is coming up again and again, most commonly because the presence of a <link> to something is the primary directional signal for the dependency graph and we use it to help generate things like the push-manifest.json etc.

Right now bundler's model has no concept for whether to link to a particular bundle or not-- basically the first time the bundling code encounters a link in the DOM of a file that points to a dependency that's inlined into a different bundle, it rewrites the link to point to that bundle and then hides/removes any subsequent links it encounters that would wind up at that bundle. This is notionally referred to as 'visited' in the code and is the only way currently that the bundling operation decides to remove an import link when it comes upon one.

My plan is to define something of Set property (either called prerequisites or stripExcludes or stripImports) on the Bundle class which will contain references to other bundles or urls which should have imports stripped, similar to the old vulcanize stripExcludes argument, except that this is per-bundle, as a general mechanism to strip links such as this out and I will update the shell merge strategy to update the property on produced bundles.

@usergenic
Copy link
Contributor Author

w00t! the fix is in. about to make release PR for bundler 2.2.0

@aomarks aomarks transferred this issue from Polymer/polymer-bundler Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants