yeoman / generator-polymer

Scaffold out a Polymer project
http://polymer-project.org
926 stars 149 forks source link

Move script into dom-module (preferred location) #194

Closed tony19 closed 9 years ago

tony19 commented 9 years ago

The generator placed the <script> tag in the root of the document, while Polymer 1.0 documentation currently shows it only inside of the <dom-module>.

While it's functionally equivalent (as far as I'm aware) to place <script> in either location, the supposedly new "preferred location" is inside the <dom-module> based on Mauro Solcia's comment on a Polymer G+ post:

A small note is to put the <script> inside the <dom-module> tag, as it's the new preferred way to improve code readability/feeling of encapsulation (documentation is being updated accordingly).

This patch moves <script> into <dom-module> for the reason mentioned above and to maintain consistency with official Polymer documentation.

robdodson commented 9 years ago

LGTM, it looks like PSK has also updated to this pattern.

arthurvr commented 9 years ago

@robdodson I think we shouldn't edit templates here and redirect all suggestions about the template files to PSK. Even if the change was already done there, we should just wait till it released. I don't want us getting in the bad habit of maintaining two versions of exactly the same codebase.

robdodson commented 9 years ago

@arthurvr I agree but in this case I wanted these changes to the yo polymer:el subgenerator. I've been meaning to make those updates but haven't gotten around to it. Otherwise yes, all template stuff will come from PSK

arthurvr commented 9 years ago

I agree but in this case I wanted these changes to the yo polymer:el subgenerator.

Yes, but this updated more than that. By the way, we isn't PSK a submodule anymore? Or even better, why doesn't it just download the latest release itself, like the WSK generator does?

robdodson commented 9 years ago

Yes, but this updated more than that

I'm aware of that, but those changes won't roll out. Next time PSK updates they'll be replaced with the PSK versions. It was quicker to just merge the PR and update those files with PSK 1.0.x when it lands.

By the way, we isn't PSK a submodule anymore? Or even better, why doesn't it just download the latest release itself, like the WSK generator does?

When I published the update to npm it seemed like the submodule didn't get bundled up so I replaced it with the zipped version because I was about to hop on a plane and disappear for a week :)

Or even better, why doesn't it just download the latest release itself, like the WSK generator does?

Can you point me to the version you're referring to? I'd be keen to try this out as an alternative to the submodule if it seems like a good approach

arthurvr commented 9 years ago

Can you point me to the version you're referring to? I'd be keen to try this out as an alternative to the submodule if it seems like a good approach

The code is in generator-mobile. I don't think it's ideal but it doesn't force the generator to release every time PSK releases.

robdodson commented 9 years ago

it doesn't force the generator to release every time PSK releases

hm yeah I've been thinking about this a bit. On the one hand, if folks don't update their generator frequently then they won't get the latest PSK, on the other hand, if at some point PSK adds something that the generator isn't expecting it could break. At least in the current form, each release of the generator is pretty much guaranteed to work with the version of PSK it ships with. @addyosmani do you have an opinion on which approach you'd prefer?

addyosmani commented 9 years ago

My two cents are that we could take advantage of https://github.com/yeoman/update-notifier here. When a user tries using the generator, we can do period checks against the version and inform them if a new version of the generator (and supported version of PSK) are available. They can then decide whether or not to make the jump. We can remember their decision and ultimately, if they decide to stick with an older version that's up to them.