New: Add mixin - Githubissues

PolymerElements / app-localize-behavior

Polymer behaviour to help internationalize your application

48 stars 55 forks source link

New: Add mixin #106

Closed stramel closed 7 years ago

stramel commented 7 years ago

A bit of a hacky consolidation but is a similar approach to how I handled it in lazy-imports.

Base and Behavior are still ES5 Mixin is ES6

Cleaned up some of the local cache similarly to how it is is handled in lazy-imports.
Moved external dependency to a separate html import
Bumped WCT to 6 on the mainline

Things to do if we decide to move forward:

[ ] Tests
[ ] Demos
[ ] Documentation
[ ] Linting to ensure no ES6 leaks into the Base or Behavior

@notwaldorf @e111077

stramel commented 7 years ago

Just checking if you guys have a chance to take a look at this...

I am currently using this mixin in my Polymer 2.0 app and haven't noticed anything strange with it.

e111077 commented 7 years ago

I don't know how I feel about having a 2.0-only / ES6 mixin in a "hybrid repository". @notwaldorf I feel like this is more your territory

notwaldorf commented 7 years ago

Thanks for the PR! Unfortunately, we don't really have a plan to convert the hybrid elements to pure 2.0 at this time, since we don't have a lot of bandwidth for that. Further, this wouldn't add too much value, since it's not actually fixing anything that's broken, and just adds another breaking change release that needs to be maintained separately :(

stramel commented 7 years ago

@notwaldorf How would this be a breaking change? I maintained backwards compability and added a new feature (mixin). The benefit is that I don't have to pull in the full legacy polymer to use this in a pure 2.0 application.

notwaldorf commented 7 years ago

I see several problems with this approach:

In a Polymer 1 app, I will pull this new (minor patch) code, and get a new file, app-localize-mixin.html, which is completely unusable in my app. I don't know how I feel about that, especially that in the future, you might try to add 2.0-only code to it, thus making this 2.0 release of this element behave _very- differently in Polymer 1 and Polymer 2 apps (and defeating the point of hybrid elements)
given my current app, after this patch, I am doing an extra file download every time I just want to import the behaviour (and you're relying on bundling or a build step to make this go away).
there is code duplication that historically for elements has been a disaster: for example, the docs for language are live both in app-localize-behavior.html and app-localize-mixin.html, and i can guarantee you future PRs will only touch one of these files, and the reviewer has to remember that it's in two.

stramel commented 7 years ago

Okay, I understand some of these pain points you are talking about. I have handled all of these in lazy-imports. I'm okay with holding off on this for now until you guys decide to make the cut over. I will keep my branch open for anyone who wants to use it.