Skip to content
This repository has been archived by the owner on May 1, 2019. It is now read-only.

Add support for lazy-imports of iron-lazy-pages #600

Closed
wants to merge 1 commit into from

Conversation

TimvdLippe
Copy link
Contributor

@rictic So I was really excited about this PR and did not want to wait till my weekend 😂

This pull request introduces lazy-import support for the analyzer. It takes the iron-lazy-pages definition and can determine what the lazy imports are (without extra rel="lazy-import" tags).

I have added 2 tests to verify the correct behavior. One is a unit test which just uses DomModuleScanner. The second one is an integration test, which I think works correct now. I have yet to try this out on a real application, but the edges/loading behavior of the analyzer is still vague for me.

Please let me know what you think!

  • CHANGELOG.md has been updated

@justinfagnani
Copy link
Contributor

@TimvdLippe I have to think about this one. Theoretically any element could be using imports as lazy, and we can't add support for all of them. My initial reaction is that I'd rather have one pattern of specifying lazy imports, and rework iron-lazy-pages to use lazy imports.

I see this as similar to the ES6 import statement vs dynamic import(). Bundlers can treat dynamic import() as lazy edges, but really any function could ultimately pass an argument to a dynamic import(). It's unlikely that a bundler will add support for every library's import wrapper.

It'd be good to get issues going for features like this so we can discuss before PR time, but thanks as always for the contributions!

@justinfagnani
Copy link
Contributor

@TimvdLippe is there a way you could change iron-lazy-pages so that instead of a URL on the page it takes a lazy-import group name. Then it would have support with no changes to the Analyzer.

@TimvdLippe
Copy link
Contributor Author

@justinfagnani Ah yeah sorry, I extensively discussed this particular feature with @rictic on both Slack on GitHub. This morning I got very excited and actually built this PR, without realising that it needs a proper introduction too 😄

A previous discussion was in a shop PR: Polymer/shop#114 (comment) The essence comes down to the following: with the current solutions, users have to copy information a lot of the times. There are hidden dependencies between dynamic importHref, fragments specified in the polymer.json and now a third one is introduced regarding rel="lazy-import". In several of our websites, we already hit the problem of forgetting imports, misspelling them, etc... We sometimes found out stuff was broken AFTER we pushed it to production.

With a single (statically analyzable) source of truth, the user experience is a lot better. There is less room for error, the intent is a lot clearer and the tools can automatically use the source of truth.

This particular pull request showcases what one of the possibilities is. I never had the intention to integrate iron-lazy-pages support into the official tooling, but given the discussion with @rictic and the small impact of the change I wanted to explore the idea. The ideal solution would be to be able to add scanners as a user that would be able to analyze arbitrary data. Then, I can export these functions to a separate scanner and publish it as a npm package and users can use it in their gulpfile.js.

Still, I am convinced that this PR serves a better experience for users using the various tools, although I would love to abstract away the hidden dependency on my iron-lazy-pages. These are my 2 cents 😉

@justinfagnani
Copy link
Contributor

Hey @TimvdLippe thanks for the explanation.

I'm still convinced that we need to a basic set of primitives for describing the dependency graph, at least for the built-in functionality. Basically, I agree with the single-source-of-truth principle, and it should be lazy-imports. lazy-imports will be able to completely replace fragments when plumbed through the Bundler.

iron-lazy-pages as is adds implicit edges to the dependency graph and thus cannot work properly with the Analyzer without changing the Analyzer, and I'd consider something to be addressed in iron-lazy-pages by making it work with the primitives that we support.

I'd much rather see iron-lazy-pages redesigned to work with lazy imports so that you use it like so:

<dom-module id="my-element">
  <link rel="lazy-import" group="foo" href="foo.html">
  <template>
    <iron-lazy-pages attr-for-selected="data-route" selected="{{route}}">
      <template is="iron-lazy-page" data-route="foo" group="foo">
        Foo page
      </template>
    </iron-lazy-pages>
  </template>
</dom-module>

The problem with adding support for iron-lazy-pages directly to the Analyzer is that we would have to do the same for any other element that defined implicit lazy edges in the dependency graph, when I'd rather point out the correct pattern to follow to get analysis and bundling support automatically.

@TimvdLippe
Copy link
Contributor Author

@justinfagnani I definitely agree that the lazy import is better than the fragments in polymer.json. The concern I have is that it very much bloats the code. For example, if you have 25 routes in your application, you would need to duplicate this for every route, having at least 50 lines of code for essentially the same thing. In our experience, we hit the problem of forgetting routes, misspelling and not discovering the error at build time. Even if iron-lazy-pages would be rewritten to use this lazy-imports, I think the user experience is worse and harder to maintain all routes.

In essence, the pattern used in Shop and PSK is that the imports are based on the route name, and invoked via importHref. The analyzer can not handle this pattern, thus requires the lazy-imports to be specified. This pattern forces a certain folder structure, which might not be possible. E.g. sometimes you want to load a page which is actually a web-component in the bower_components. This is one of the cases where the flexibility of iron-lazy-pages helps. It does not dictate any pattern and leaves it up to the user to determine 1. which routes are actually imported, thus not requiring a <my-404> instead of a simple 404 section and 2. where the actual files are stored, maybe it is a dependency or directly in src/ or even in a sub-directory of src/ all together.

As I said, I would like to refrain from adding official support for iron-lazy-pages in the analyzer. This PR was merely a showcase of how easy it would be to add support in. My preference would be that the analyzer can accept more scanners (or extension of scanners) that can process the scanned dom module, before the edges are resolved. E.g. after scanning, but before resolving.

@justinfagnani
Copy link
Contributor

@TimvdLippe thanks for the interesting PR. I'm cleaning up some things now, so I'm going to close this, but I'm sure we'll discuss this type of feature in issues soon.

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

Successfully merging this pull request may close these issues.

2 participants