Better handle external JS files referenced via script tags

[rictic]

So, we have this really terrible hack, whereby we generate a new Document for a js file when it is referenced in an external script tag in an HTML document. We do this so that we can inject an artificial import of the HTML document into the js document, so that the HTML document's dependencies are also dependencies of the js document.

This results in duplicate top level documents for the same url, which is a violation of our model and that we have to do hacky workarounds for.

A much cleaner solution would be to produce modified ScannedDocuments for all scripts included via script tags and use them for resolution. The tricky part is that we would not want to cache those scanned documents in the scanned document cache, as they're not file local.

@justinfagnani for thoughts

[rictic]

And no matter what the solution we go with, we need more tests for this use case.

[justinfagnani]

@rictic I'm having trouble paging in the actual problem here. We used to add the HTML document that imported some script as a feature of the script document. This was a little odd, so @usergenic fixed it to inject a synthetic import as a feature instead of the HTML document directly, in #734.

But there were always two documents in these scenarios: the HTML and JS. When was a duplicate top-level doc created in the situation you're talking about?

[rictic]

I've forgotten about this too. This is the workaround we have in place: https://github.com/Polymer/polymer-analyzer/blob/master/src/model/analysis.ts#L139

The issue is introduced because we call new Document here: https://github.com/Polymer/polymer-analyzer/blob/master/src/html/html-script-tag.ts#L54

But that's a layering violation, and worse, AnalysisContext.getDocument is going to create that same document.

I don't remember exactly why we can't get the document from the analysis context via document._analysisContext.getDocument rather than creating a duplicate. Hm, oh, because we then mutate the document, which is illegal under our caching constraints.

[justinfagnani]

Ok, interesting, I get it now.

To work well with the cache the synthetic import really should be owned by the HTML document, but appear as part of the JS document.

I wonder if, rather than inventing a way to induce features on another document (are there any other use cases for that?), we should just make a special carve-out for this by having getFeature also look up a context document and treat that as imported. Then we don't ever need to mutate any documents, we just need a map of context documents keyed by the document that a context is provided for.

[rictic]

That works when you have the full analysis, but sometimes you just have the Document. I guess you always get a Document from an Analysis, so it does seem like we should be able to do something at that point.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.