Issue 211393006: Enables codegen support in polymer

Issue 211393006: Enables codegen support in polymer (Closed)

Created:
6 years, 9 months ago by Siggi Cherem (dart-lang)

Modified:
6 years, 9 months ago

Reviewers:
Jennifer Messerly, blois

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Enables codegen support in polymer R=blois@google.com, jmesserly@google.com Committed: https://code.google.com/p/dart/source/detail?r=34531

Patch Set 4 : #

Created: 6 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+1235 lines, -249 lines)			Patch
M	pkg/code_transformers/lib/src/resolver.dart	View	1 2	2 chunks	+7 lines, -0 lines	0 comments	Download
M	pkg/code_transformers/lib/src/resolver_impl.dart	View	1 2 3	3 chunks	+10 lines, -2 lines	0 comments	Download
M	pkg/polymer/CHANGELOG.md	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	pkg/polymer/lib/deploy.dart	View	1 2	5 chunks	+17 lines, -13 lines	0 comments	Download
M	pkg/polymer/lib/src/build/common.dart	View	1 2	1 chunk	+24 lines, -0 lines	0 comments	Download
M	pkg/polymer/lib/src/build/linter.dart	View	1 2	4 chunks	+6 lines, -29 lines	0 comments	Download
M	pkg/polymer/lib/src/build/mirrors_remover.dart	View		1 chunk	+3 lines, -2 lines	0 comments	Download
M	pkg/polymer/lib/src/build/script_compactor.dart	View	1 2	5 chunks	+538 lines, -134 lines	0 comments	Download
M	pkg/polymer/lib/transformer.dart	View		2 chunks	+4 lines, -3 lines	0 comments	Download
M	pkg/polymer/pubspec.yaml	View	1 2	2 chunks	+8 lines, -11 lines	0 comments	Download
M	pkg/polymer/test/build/all_phases_test.dart	View		8 chunks	+23 lines, -6 lines	0 comments	Download
M	pkg/polymer/test/build/common.dart	View	1 2	3 chunks	+48 lines, -2 lines	0 comments	Download
M	pkg/polymer/test/build/script_compactor_test.dart	View	1 2	7 chunks	+486 lines, -18 lines	0 comments	Download
M	pkg/polymer/test/prop_attr_reflection_test.dart	View	1 2	1 chunk	+7 lines, -7 lines	0 comments	Download
M	pkg/smoke/pubspec.yaml	View	1 2 3	1 chunk	+6 lines, -8 lines	0 comments	Download
A	pkg/template_binding/CHANGELOG.md	View	1 2	1 chunk	+16 lines, -0 lines	0 comments	Download
M	pkg/template_binding/lib/src/mustache_tokens.dart	View	1 2 3	3 chunks	+14 lines, -8 lines	0 comments	Download
M	pkg/template_binding/lib/src/template_iterator.dart	View	1 2 3	4 chunks	+14 lines, -4 lines	0 comments	Download
M	pkg/template_binding/pubspec.yaml	View	1 2	2 chunks	+2 lines, -2 lines	0 comments	Download

Messages

Total messages: 9 (0 generated)

Expand Messages | Collapse Messages

Siggi Cherem (dart-lang)

At last, all tests are passing and TodoMVC as well. I haven't tried it yet ...

6 years, 9 months ago (2014-03-26 01:18:09 UTC) #1

blois

Thoughts on supporting deferred libs? The generated code probably needs to support being injected at ...

6 years, 9 months ago (2014-03-26 16:02:33 UTC) #2

Siggi Cherem (dart-lang)

Thanks Pete, Good idea on supporting deferred libraries. I'll look into it, but I'll probably ...

6 years, 9 months ago (2014-03-26 17:56:34 UTC) #3

Thanks Pete,

Good idea on supporting deferred libraries. I'll look into it, but I'll probably
keep it as a separate change.

https://codereview.chromium.org/211393006/diff/30001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/common.dart (right):

https://codereview.chromium.org/211393006/diff/30001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/common.dart:192: // These names have meaning in SVG or
MathML, so they aren't allowed as custom
On 2014/03/26 16:02:33, blois wrote:
> Is this list from somewhere?

yeah, that's what the link below in [isCustomTagName] was originally pointing
to. Seems that the link was redirected, but the #tag was not preserved. I
updated the link to reflect the latest location.

(FYI this code was just refactored from another file, nothing new here)

https://codereview.chromium.org/211393006/diff/30001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/script_compactor.dart (right):

https://codereview.chromium.org/211393006/diff/30001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:215: _customTagElement =
_lookupType(polymerLib, 'CustomTag');
On 2014/03/26 16:02:33, blois wrote:
> If you change this to be the constructor then can just check the attribute
> against that.

Nice idea. Done.

https://codereview.chromium.org/211393006/diff/30001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:217: _observableElement =
_lookupType(observeLib, 'ObservableProperty');
On 2014/03/26 16:02:33, blois wrote:
> Why not use the const TopLevelVariable?

good question. I'm on the fence on this.

I mainly have it to allow future subclasses of these types too (runQuery checks
for annotations that are are instances of this type). For example, if users want
to define their own subtype of @observable/@published, they can.

There are not uses of this today, so it's a good question whether we should do
the query this way or not.

I was motivated by what we've done so far, though. Because @observable extends
from @reflectable, and @published extends from @observable, we can simply run
queries about @observable and these will apply automatically to @published.

The main place where I think this would be really useful, would be if we switch
the observe package to use the resolver, that would help remove that odd
dependency here:
https://github.com/dart-lang/bleeding_edge/blob/master/dart/pkg/observe/lib/t...

https://codereview.chromium.org/211393006/diff/30001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:403: return args[0].stringValue;
On 2014/03/26 16:02:33, blois wrote:
> Might want to check that this is a StringLiteral- just in case it's referring
to
> a const elsewhere.

Done.

https://codereview.chromium.org/211393006/diff/30001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:548: // value may be 'a b c' or
'a,b,c'
On 2014/03/26 16:02:33, blois wrote:
> how about a, b c (I believe that polymer.js supports this)?
> 
> value.split(new RegExp('\s|,'))
> or:
> value.split(',').expand((v) => v.split(' '))

good catch. I had copied this from the linter, without realizing that it was
different in the runtime library. Your regexp matches what we do at runtime too,
so I went with that.

Jennifer Messerly

This is so cool. Better tree shaking with HTML! it looks great overall, only two ...

6 years, 9 months ago (2014-03-27 02:20:32 UTC) #4

Siggi Cherem (dart-lang)

Thanks John! These were great suggestions! PTAL https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/deploy.dart File pkg/polymer/lib/deploy.dart (right): https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/deploy.dart#newcode29 pkg/polymer/lib/deploy.dart:29: import 'transformer.dart'; ...

6 years, 9 months ago (2014-03-28 01:04:25 UTC) #5

Thanks John!

These were great suggestions!

PTAL

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/deploy.dart
File pkg/polymer/lib/deploy.dart (right):

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/deploy.d...
pkg/polymer/lib/deploy.dart:29: import 'transformer.dart';
On 2014/03/27 02:20:32, John Messerly wrote:
> move up by other package: imports?

Done.

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/deploy.d...
pkg/polymer/lib/deploy.dart:64: BarbackOptions _createTestOptions(String
testFile, String outDir,
On 2014/03/27 02:20:32, John Messerly wrote:
> off topic, but I wonder how long until we can use "pub build" or "pub serve"
> instead of _createTestOptions...

not clear yet. There is still a lot of work to be done in the test
infrastructure before we are there.

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/deploy.d...
pkg/polymer/lib/deploy.dart:127: new Directory(dir).listSync(recursive: false,
followLinks: false)
On 2014/03/27 02:20:32, John Messerly wrote:
> fyi -- recursive: false is the default:
>
https://api.dartlang.org/apidocs/channels/stable/#dart-io.Directory@id_listSync

thx, done

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/linter.dart (right):

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/linter.dart:404: final _ATTRIBUTES_REGEX = new
RegExp(r'\s|,');
On 2014/03/27 02:20:32, John Messerly wrote:
> maybe comment that this is identical to lib/src/declaration.dart? or could we
> move into a shared file somehow?

Done. added the reference and shared this with the one in script_compactor, but
I rather leave the one in declaration.dart separate for now.

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/script_compactor.dart (right):

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:43: final Resolvers resolvers;
On 2014/03/27 02:20:32, John Messerly wrote:
> this is an issue with the code_transformer pkg, but ... as a general rule it's
> confusing to have a type named "Resolvers" and "Resolver" in the same system.
> What do you call a list of Resolvers? And which type was I just talking about?
> Exactly. ;-)
> 
> (not sure if I first saw this from the .NET design guidelines or something
much
> older, but it's a good rule to follow...)
>  

Yeah, that's a good point.

Just chatted with Pete and we had some ideas. One idea is to rename it to
ResolverProvider, ResolverFactory, or ResolverCache.

Meanwhile, I filed: https://code.google.com/p/dart/issues/detail?id=17864

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:95: ClassElement
_htmlElementElement;
On 2014/03/27 02:20:32, John Messerly wrote:
> A thought here: there's a lot of state in this class. it went from ~10 fields
to
> ~22 ...  and much of it is mutable. I wonder if some of it could be collected
> into other objects?
> 
> an idea: maybe all of these constant types could be pulled out into a class

good idea. I moved these out in to a separate class, and inlined a couple that
didn't need to be a field. I thought at some point we might want to split out
the analysis of Dart code into a separate class or library, but we can do that
as a follow up later.

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:113: final SmokeCodeGenerator
generator = new SmokeCodeGenerator();
On 2014/03/27 02:20:32, John Messerly wrote:
> IMO, it's nice to not repeat the type annotation for final fields, if
> possible...

Done.

It seems fine in this particular example, but I'm not convinced about making
this a general rule. Here it seems fine because we only use the field within
this file, but for a public API I think keeping the annotation can improve
documentation and type checking (if someone were to use the class as an
interface, for example.)

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:200: var polymerLib =
r.getLibrary(new AssetId('polymer', 'lib/polymer.dart'));
On 2014/03/27 02:20:32, John Messerly wrote:
> just curious, any reason to prefer AssetIds over package: URIs or vice versa?
> 
> (seems like there's two very similar ways to query for libraries)

code_transformers indexes the libraries that are reachable from the program
using the asset-ids, so lookup by asset id is more efficient. Lookup-by-uri has
to search all of the assets to find which one matches the import uri. We only
use it for core libraries that can't be indexed with asset-ids.

I filed a bug to consider adding a secondary index by uri, though
(https://code.google.com/p/dart/issues/detail?id=17865)

Funny enough, there is not just 2, but 3 ways to query for libraries! (you can
query them by name too :-))

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:231: throw new
StateError("Internal error in polymer-builder: couldn't find "
On 2014/03/27 02:20:32, John Messerly wrote:
> could this error be cased by the user forgetting to import polymer anywhere?
Or
> is it firmly in the "should never happen" category?

It was happening in our tests before I added the mock libraries, but other than
that it shouldn't happen in user's code. If this happens, it means they have not
imported polymer, not even to call the initialization.

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:245: void
_extractUsesOfMirrors(_) {
On 2014/03/27 02:20:32, John Messerly wrote:
> Just wanted to say this method is awesome. It reads like psuedocode. :)

:)

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:266: // remove the warning
below?
On 2014/03/27 02:20:32, John Messerly wrote:
> yes, please, let's remove :)
> 
> (even for Dart, things can be document.register'd ... more comment below)

Done.

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:323: /// Process a class
([cls]). If it contains an appropriate [CustomTag]
On 2014/03/27 02:20:32, John Messerly wrote:
> so, today I can have a Dart class, mark it @reflectable (or @MirrorsUsed) and
> then document.register it sometime later. This is great for things like
deferred
> libraries or just generally the app wants to lazy load things.
> 
> It seems like this new world requires @CustomTag.
> 
> Since the features are all Polymer features, would it be possible instead to
> check for the Polymer mixin? That should be a 100% reliable signal that it is
a
> Polymer element & has the framework features. 
> 
> (Another issue: @CustomTag is currently implemented by Polymer, but I think we
> want it to be a more general feature someday if possible...)

This is a great idea! I made the switch.

The only thing missing is what to do for codegen when people have the following:
 - they use 'attributes' in the html
 - they manually call Polymer.register
 - they do not include @published

Here is where I think @UnregisteredCustomTag could be useful, or we can tell
them just to put the @published. I'm ok to start with the latter but we might
want to still have the former too. Thoughts?

I updated the bug with these details too.

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:346: logger.error('@CustomTag is
no longer supported on private classes:'
On 2014/03/27 02:20:32, John Messerly wrote:
> just curious, is this something we could lift in the future?
> 

(Note that this warning was there before this change too)

Yes, we could lift this in the future, but we would have to change how we
generate initialization code.

The reason we have the restriction today is that we generate the initializers in
the main entrypoint file, so we need to be able to import this class. The fix
would be to change things so instead of doing:

   bootstrap.dart:
     configureForDeployment([
       () => Polymer.register('tag-name', prefix.ClassName),
     ]);

we would do:

   bootstrap.dart:
     configureForDeployment([
       prefix.polymer_init,
     ]);

   this_file.dart:
     ...
     polymer_init() => Polymer.register('tag-name', _ClassName);

> I guess we can always recommend putting types in a private ("src") library
> instead. Maybe we could suggest that in the error message?

I agree we should give hints on what to do, so I rewrote the warning.

I'm not sure if using a private library will work because the type needs to be
visible from importing this library (we only register things that are in the
immediate libraries from <script> tags, not in transitive imports.)

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:353: recorder.lookupMember(cls,
'registerCallback');
On 2014/03/27 02:20:32, John Messerly wrote:
> this reminds me, #registerCallback needs to go away :-)
> 
> (but hey, it's a nice place to issue a deprecation warning in a future CL)

it's actually funny because smoke initially didn't have everything to support
#registerCallback and *Changed below, I had to add more features to support
them... oh well :)

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:405: logger.error('Only string
literals are currently supported by the polymer'
On 2014/03/27 02:20:32, John Messerly wrote:
> totally fine to fix in a follow up, but it looks like ConstantEvaluator in
> lib/src/generated/constants.dart might do what you want pretty easily:
> 
> var result = new ConstantEvaluator(source, typeProvider).evaluate(args[0]);
> if (result.isValid && result.value is String) { 
>  ... use result.value
> } else {
>  ... print "not a constant" or "not a string" error
> }

nice! Done

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:457: // classes are initialized
first?
On 2014/03/27 02:20:32, John Messerly wrote:
> currently no -- Polymer.js will handle it using the "extends" chain.
> 
> Is the order otherwise well defined? One nice property of html imports is it's
> pretty easy to infer the order (* assuming the import graph isn't super deep)

yeah, we should be preserving the same order here, the only thing that changed
with this CL is that it used to be interleaved Polymer.register and initMethods
depending on the order they appeared in the source code. Now we cluster all
initMethods (for a given library) together first, and do class registrations
together afterwards.

I had this in the reverse order in the previous patch (classes before
initMethods), but I just swapped them to match the order we have in
mirrors_loader.dart. Yay consistency!

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:579: isTwoWay = !isEvent &&
bindings.isWhole && (isCustomTag ||
On 2014/03/27 02:20:32, John Messerly wrote:
> how important is this?
> 
> it worries me on the maintenance side, new ones pop up occasionally in
NodeBind
> and/or TemplateBinding. e.g. it used to have some bindings for "template". I
> think "ref" might still be bindable.
> 
> But also, with custom elements we can't really know; the element might chose
> one-way or two-way.
> 
> Would it be a bad idea to defensively assume two-way bindings?

We could, not sure. We'd have to measure how much more code gets generated.

We are already defensively assuming two-way on any custom-tag attribute, though.
So I'm inclined to try this out white listing the possible two-way bindings we
have in TemplateBinding/NodeBind.

One idea: could we make the white list part of template_binding? Then it might
be easier to keep it up to date when updating that package.

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:663: // we share some code here
and in template_binding/src/mustache_tokens.dart?
On 2014/03/27 02:20:32, John Messerly wrote:
> could you just import that file? it doesn't look like it depends on dart:html
or
> anything crazy. I'm happy to help restructure it if needed...
> 
> this code has changed a few times already so that makes me leery about having
a
> copy

Done. I managed to refactor it slightly and now I'm using it here. It
transitively imported dart:html via template_binding because of the dependency
on BindingDelegate, so I was hesitant at first to do it, but the changes seemed
pretty small. Let me know what you think.

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/test/build/s...
File pkg/polymer/test/build/script_compactor_test.dart (right):

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/test/build/s...
pkg/polymer/test/build/script_compactor_test.dart:241: #anotherAttribute:
r'anotherAttribute',
On 2014/03/27 02:20:32, John Messerly wrote:
> not related to this CL but, I wonder if we should try and standardize on
Strings
> in smoke, so we can avoid  the code size of the extra table & perf hit of
> conversion back and forth...
> 
> (IIRC, Symbols were invented specifically to help dart2js mirrors, and that
> isn't applicable to Polymer once we're using pkg:smoke)

Good point. It would be totally doable. That will probably hide most places
where we do conversion from name-to-symbol to be hidden inside
smoke/mirrors.dart.

Should we then switch package:observe to use string instead of symbol?

I created a bug so we don't forget: dartbug.com/17863

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/test/prop_at...
File pkg/polymer/test/prop_attr_reflection_test.dart (right):

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/test/prop_at...
pkg/polymer/test/prop_attr_reflection_test.dart:46: // TODO(sigmund): add a
different annotation for this? (dartbug.com/17790)
On 2014/03/27 02:20:32, John Messerly wrote:
> you can remove this if looking for Polymer mixin :-)

Done :) 

I changed above the @observable to @published though. Technically @observable
works because we are treating them both the same way in the codegen, but in the
future I expect we'll be doing something different for each.

https://codereview.chromium.org/211393006/diff/130001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/script_compactor.dart (right):

https://codereview.chromium.org/211393006/diff/130001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/script_compactor.dart:666: List<ClassElement>
_visibleClassesOf(LibraryElement lib) {
FYI - there were no changes here, I just moved these down when I realized that
they are static.

Pete - let me know if you'd like me to move these to code_transformers at some
point.

https://codereview.chromium.org/211393006/diff/130001/pkg/template_binding/CH...
File pkg/template_binding/CHANGELOG.md (right):

https://codereview.chromium.org/211393006/diff/130001/pkg/template_binding/CH...
pkg/template_binding/CHANGELOG.md:15: [fcb7a5]:
https://github.com/Polymer/TemplateBinding/commit/fcb7a502794f19544f2d4b77c96...
I'm not 100%, I took this commit number from the last update you did in this
file:
https://github.com/dart-lang/bleeding_edge/blob/master/dart/pkg/template_bind...

is that right?

https://codereview.chromium.org/211393006/diff/130001/pkg/template_binding/li...
File pkg/template_binding/lib/src/mustache_tokens.dart (right):

https://codereview.chromium.org/211393006/diff/130001/pkg/template_binding/li...
pkg/template_binding/lib/src/mustache_tokens.dart:119: if (delegateFn == null) {
this was a patch Justin did upstream, I wanted to do it here too to prevent
failures from trying to parse a pol-exp as a prop-path.

Jennifer Messerly

LGTM https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/build/script_compactor.dart File pkg/polymer/lib/src/build/script_compactor.dart (right): https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/build/script_compactor.dart#newcode323 pkg/polymer/lib/src/build/script_compactor.dart:323: /// Process a class ([cls]). If it contains ...

6 years, 9 months ago (2014-03-28 21:48:35 UTC) #6

LGTM

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/script_compactor.dart (right):

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:323: /// Process a class
([cls]). If it contains an appropriate [CustomTag]
On 2014/03/28 01:04:26, Siggi Cherem (dart-lang) wrote:
> On 2014/03/27 02:20:32, John Messerly wrote:
> > so, today I can have a Dart class, mark it @reflectable (or @MirrorsUsed)
and
> > then document.register it sometime later. This is great for things like
> deferred
> > libraries or just generally the app wants to lazy load things.
> > 
> > It seems like this new world requires @CustomTag.
> > 
> > Since the features are all Polymer features, would it be possible instead to
> > check for the Polymer mixin? That should be a 100% reliable signal that it
is
> a
> > Polymer element & has the framework features. 
> > 
> > (Another issue: @CustomTag is currently implemented by Polymer, but I think
we
> > want it to be a more general feature someday if possible...)
> 
> This is a great idea! I made the switch.
> 
> The only thing missing is what to do for codegen when people have the
following:
>  - they use 'attributes' in the html
>  - they manually call Polymer.register
>  - they do not include @published
> 
> Here is where I think @UnregisteredCustomTag could be useful, or we can tell
> them just to put the @published. I'm ok to start with the latter but we might
> want to still have the former too. Thoughts?
> 
> I updated the bug with these details too.

ah, yeah that makes sense. seems like several problems all tangled up, including
the "attributes= vs @published", the fact that @CustomTag is current polymer
specific (but probably shouldn't be), @CustomTag not being able to register the
same type for multiple tag names, reflectability vs @published ... etc :). I
like where this ended up tho but agree we should have another round of design
here

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:405: logger.error('Only string
literals are currently supported by the polymer'
On 2014/03/28 01:04:26, Siggi Cherem (dart-lang) wrote:
> On 2014/03/27 02:20:32, John Messerly wrote:
> > totally fine to fix in a follow up, but it looks like ConstantEvaluator in
> > lib/src/generated/constants.dart might do what you want pretty easily:
> > 
> > var result = new ConstantEvaluator(source, typeProvider).evaluate(args[0]);
> > if (result.isValid && result.value is String) { 
> >  ... use result.value
> > } else {
> >  ... print "not a constant" or "not a string" error
> > }
> 
> nice! Done

it worked? awesome! :D

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:579: isTwoWay = !isEvent &&
bindings.isWhole && (isCustomTag ||
On 2014/03/28 01:04:26, Siggi Cherem (dart-lang) wrote:
> On 2014/03/27 02:20:32, John Messerly wrote:
> > how important is this?
> > 
> > it worries me on the maintenance side, new ones pop up occasionally in
> NodeBind
> > and/or TemplateBinding. e.g. it used to have some bindings for "template". I
> > think "ref" might still be bindable.
> > 
> > But also, with custom elements we can't really know; the element might chose
> > one-way or two-way.
> > 
> > Would it be a bad idea to defensively assume two-way bindings?
> 
> We could, not sure. We'd have to measure how much more code gets generated.
> 
> We are already defensively assuming two-way on any custom-tag attribute,
though.
> So I'm inclined to try this out white listing the possible two-way bindings we
> have in TemplateBinding/NodeBind.
> 
> One idea: could we make the white list part of template_binding? Then it might
> be easier to keep it up to date when updating that package.

yeah, it would be nice to be in template_binding, ideally even a way we can
assert that the binding is in the list at creation time. that way we can prevent
bugs. I don't think our Polymer tests adequately cover template_binding (whereas
template_binding tests cover all of these).

Doing it in a follow up change is good for me tho.

https://codereview.chromium.org/211393006/diff/130001/pkg/template_binding/li...
File pkg/template_binding/lib/src/mustache_tokens.dart (right):

https://codereview.chromium.org/211393006/diff/130001/pkg/template_binding/li...
pkg/template_binding/lib/src/mustache_tokens.dart:10: // of tempalte_binding.
This allows using this code in command-line tools as
typo: template_binding

Siggi Cherem (dart-lang)

Thanks John! https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/build/script_compactor.dart File pkg/polymer/lib/src/build/script_compactor.dart (right): https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/build/script_compactor.dart#newcode405 pkg/polymer/lib/src/build/script_compactor.dart:405: logger.error('Only string literals are currently supported by ...

6 years, 9 months ago (2014-03-28 22:08:47 UTC) #7

Thanks John!

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/script_compactor.dart (right):

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:405: logger.error('Only string
literals are currently supported by the polymer'
On 2014/03/28 21:48:35, John Messerly wrote:
> On 2014/03/28 01:04:26, Siggi Cherem (dart-lang) wrote:
> > On 2014/03/27 02:20:32, John Messerly wrote:
> > > totally fine to fix in a follow up, but it looks like ConstantEvaluator in
> > > lib/src/generated/constants.dart might do what you want pretty easily:
> > > 
> > > var result = new ConstantEvaluator(source,
typeProvider).evaluate(args[0]);
> > > if (result.isValid && result.value is String) { 
> > >  ... use result.value
> > > } else {
> > >  ... print "not a constant" or "not a string" error
> > > }
> > 
> > nice! Done
> 
> it worked? awesome! :D

yeah! I was so surprised that I had to explicitly add tests before I believed it
:)

https://codereview.chromium.org/211393006/diff/90001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:579: isTwoWay = !isEvent &&
bindings.isWhole && (isCustomTag ||
On 2014/03/28 21:48:35, John Messerly wrote:
> On 2014/03/28 01:04:26, Siggi Cherem (dart-lang) wrote:
> > On 2014/03/27 02:20:32, John Messerly wrote:
> > > how important is this?
> > > 
> > > it worries me on the maintenance side, new ones pop up occasionally in
> > NodeBind
> > > and/or TemplateBinding. e.g. it used to have some bindings for "template".
I
> > > think "ref" might still be bindable.
> > > 
> > > But also, with custom elements we can't really know; the element might
chose
> > > one-way or two-way.
> > > 
> > > Would it be a bad idea to defensively assume two-way bindings?
> > 
> > We could, not sure. We'd have to measure how much more code gets generated.
> > 
> > We are already defensively assuming two-way on any custom-tag attribute,
> though.
> > So I'm inclined to try this out white listing the possible two-way bindings
we
> > have in TemplateBinding/NodeBind.
> > 
> > One idea: could we make the white list part of template_binding? Then it
might
> > be easier to keep it up to date when updating that package.
> 
> yeah, it would be nice to be in template_binding, ideally even a way we can
> assert that the binding is in the list at creation time. that way we can
prevent
> bugs. I don't think our Polymer tests adequately cover template_binding
(whereas
> template_binding tests cover all of these).
> 
> Doing it in a follow up change is good for me tho.
> 

Sounds great. I'll file a bug to track as well.

https://codereview.chromium.org/211393006/diff/130001/pkg/template_binding/li...
File pkg/template_binding/lib/src/mustache_tokens.dart (right):

https://codereview.chromium.org/211393006/diff/130001/pkg/template_binding/li...
pkg/template_binding/lib/src/mustache_tokens.dart:10: // of tempalte_binding.
This allows using this code in command-line tools as
On 2014/03/28 21:48:35, John Messerly wrote:
> typo: template_binding

Done.

Siggi Cherem (dart-lang)

6 years, 9 months ago (2014-03-28 22:11:47 UTC) #9

Message was sent while issue was closed.

Committed patchset #4 manually as r34531 (presubmit successful).

Expand Messages | Collapse Messages