[bundler] Fixed a couple important issues regarding our use of Rollup #722
Conversation
|
Fixes #568 and hopefully others... |
… resolution doesn't mix well with absolute/resolved URLs.
usergenic
force-pushed
the
bundler-multiple-imports-of-same-fragment
branch
from
71fba41
to
2f12dc4
Compare
|
Verified the fix works for multiple |
packages/bundler/src/es6-rewriter.ts
Outdated
| } | ||
| const rollupBundle = await rollup({ | ||
| input, | ||
| external, | ||
| external: (moduleId, parentId) => { |
There was a problem hiding this comment.
Add comment like "external tells rollup not to bundle a particular import".
packages/bundler/src/es6-rewriter.ts
Outdated
| @@ -188,18 +202,50 @@ export class Es6Rewriter { | |||
| return resolutions; | |||
| } | |||
|
|
|||
| rewriteEs6SourceUrlsToResolved( | |||
| node: babel.Node, jsImportResolvedUrls: Map<string, ResolvedUrl>) { | |||
| const bundler = this.bundler; | |||
There was a problem hiding this comment.
Totes unused. Removed. Thanks!
…m:Polymer/tools into bundler-multiple-imports-of-same-fragment
packages/bundler/CHANGELOG.md
Outdated
| @@ -5,7 +5,8 @@ All notable changes to this project will be documented in this file. | |||
| The format is based on [Keep a Changelog](http://keepachangelog.com/) | |||
| and this project adheres to [Semantic Versioning](http://semver.org/). | |||
|
|
|||
| <!-- ## Unreleased --> | |||
| ## Unreleased | |||
| * Fix issue with multiple dynamic imports of the same fragment. | |||
There was a problem hiding this comment.
can you add a link to a bug?
| @@ -78,7 +78,7 @@ async function prepareBundleModule( | |||
| getOrSetBundleModuleExportName(assignedBundle, sourceUrl, '*'); | |||
| bundleSource.body.push(babel.importDeclaration( | |||
| [babel.importNamespaceSpecifier(babel.identifier(starExportName))], | |||
| babel.stringLiteral(rebasedSourceUrl))); | |||
| babel.stringLiteral(sourceUrl))); | |||
There was a problem hiding this comment.
Do we know that sourceUrl is always valid? Why did we need ensureLeadingDot on rebasedSourceUrl?
Can you add a comment to, or rename, rebasedSourceUrl? I'm not clear on the name exactly what it is.
There was a problem hiding this comment.
There should no longer be an issue of confusion here as sourceUrl is always a ResolvedUrl since it comes from for (const sourceUrl of [...assignedBundle.bundle.files].sort()) {
The "rebasedSourceUrl" was a thing back when I was trying to keep them all relative URLs but now I only use resolveUrls until the final product is delivered out of the bundler.
There was a problem hiding this comment.
I could rename it to resolvedSourceUrl if that clarifies it.
| @@ -19,6 +19,7 @@ export function getAnalysisDocument(analysis: Analysis, url: string): Document { | |||
| return result.value; | |||
| } | |||
| if (result.error) { | |||
| console.log(result); | |||
|
Have filed bug on rollup rollup/rollup#2487 |
…le output of results now that problem traced to rollup.
|
Filed another bug on rollup which is kind of blocking the ExportAllDeclaration from working properly here. rollup/rollup#2489 |
|
Proposed solution is to transform |
…ports-of-same-fragment
|
@usergenic could you release a new |
|
@usergenic This code is in polymer-bundle@4.0.3, right? I'm still having issue building with lit-html and not running into import confusion, was there something more than just an update required to get this update working? |
externalidentification by use of function in Rollup config, so we can reliably identify URLs for dynamic imports as external, so they are not inlined and/or deduplicated.