Skip to content

Fix incorrect module specifiers in polymer-3-element init template.#366

Merged
aomarks merged 2 commits intomasterfrom
fix-30-template
May 10, 2018
Merged

Fix incorrect module specifiers in polymer-3-element init template.#366
aomarks merged 2 commits intomasterfrom
fix-30-template

Conversation

@aomarks
Copy link
Copy Markdown
Member

@aomarks aomarks commented May 10, 2018

We always need to use bare module specifiers, not relative URLs, when we refer to our dependencies. That means using inline scripts with import statements, and not external script tags. In this case it caused some Polymer modules to be loaded twice, once from "/components/my-element/node_modules/@polymer/..." and again from "/components/node_modules/@polymer/...".

Also bump the minimum version of iron-demo-helpers, since I was getting an old version of prism-element that had a missing "prism-theme-default.js".

Fixes #365

We always need to use bare module specifiers, not relative URLs, when we
refer to our dependencies. That means using inline scripts with `import`
statements, and not external script tags.  In this case it caused some
Polymer modules to be loaded twice, once from
"/components/<element>/node_modules/@polymer/..." and again from
"/components/node_modules/@polymer/...".

Also bump the minimum version of iron-demo-helpers, since I was getting
an old version of prism-element that had a missing
"prism-theme-default.js".

Fixes #365
@aomarks aomarks requested a review from bicknellr May 10, 2018 21:24
@aomarks aomarks force-pushed the fix-30-template branch from 9865520 to d78d4d3 Compare May 10, 2018 21:27
@bicknellr
Copy link
Copy Markdown
Member

We always need to use bare module specifiers, not relative URLs, when we refer to our dependencies.

But we can't do this for classic scripts, right? https://github.com/Polymer/tools/pull/366/files#diff-59d2389829a2bc107f1def84055d9db2R9 Didn't we decide that we don't want to serve from /components/ anymore?

@bicknellr bicknellr requested a review from justinfagnani May 10, 2018 21:28
@bicknellr
Copy link
Copy Markdown
Member

bicknellr commented May 10, 2018

Oh, is this causing a problem when testing through WCT?
(edit: WCT still always goes through /components/ AFAIK)

@aomarks
Copy link
Copy Markdown
Member Author

aomarks commented May 10, 2018

Oh, is this causing a problem when testing through WCT?

See #365. If you init polymer-3-element, then run polymer serve, then visit the URL that it tells you to (which still includes the /components/ directory), then you get errors with double-registration of dom-module.

But we can't do this for classic scripts, right? https://github.com/Polymer/tools/pull/366/files#diff-59d2389829a2bc107f1def84055d9db2R9

Right, it only works with modules.

Didn't we decide that we don't want to serve from /components/ anymore?

Yeah, but we still print it out when you run serve. It's still needed for 2.x use-cases, which we still support, so maybe we need to do something smart to detect what URL to suggest people visit. I think we should make sure the /components/ directory works for now, though.

Copy link
Copy Markdown
Member

@bicknellr bicknellr left a comment

Choose a reason for hiding this comment

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

Ok, this seems fine to roll back until we can group it w/ changes to modulizer given that none of the other things we vend match this yet. This PR closes #219 also.

<script type="module" src="../node_modules/@polymer/iron-demo-helpers/demo-pages-shared-styles.js"></script>
<script type="module" src="../node_modules/@polymer/iron-demo-helpers/demo-snippet.js"></script>
<script type="module">
import '@polymer/iron-demo-helpers/demo-pages-shared-styles';
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

.js

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The .js isn't actually needed, node resolution includes adding a .js. Have we typically been including them anyway?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think so (modulizer doesn't remove them, fwiw) but it doesn't seem like we're totally consistent since some of our projects make use of node's main field: https://github.com/Polymer/pwa-starter-kit/blob/8da3dcc42472e99fa22d0a996362828d31dc70bb/src/components/my-app.js#L11-L13

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

IMO, we shouldn't have allowed these kinds of special behaviors from Node.js' specifier resolution in polyserve's rewriting and really hope this gets dropped in spec land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants