Issue 882823003: Adds @HtmlImport with both dynamic and transformer based implementations. Still need an HtmlInliner…

Issue 882823003: Adds @HtmlImport with both dynamic and transformer based implementations. Still need an HtmlInliner… (Closed)

Created:
5 years, 11 months ago by jakemac

Modified:
5 years, 10 months ago

Reviewers:
Siggi Cherem (dart-lang), Jennifer Messerly

CC:
reviews_dartlang.org, Siggi Cherem (dart-lang)

Base URL:
git@github.com:dart-lang/web-components.git@master

Target Ref:
refs/heads/master

Visibility:
Public.

More Reviews

Description

Adds @HtmlImport with both dynamic and transformer based implementations. Still need an HtmlInliner to inline the imports, but this will statically add them to the entry document. R=jmesserly@google.com Committed: https://github.com/dart-lang/core-elements/commit/2aaa0b548c02fd094d3903009546bf15a3984568

Patch Set 2 : format and update to use InitializerPluginTransformer #

Patch Set 3 : remove common.dart test file #

Total comments: 4

Patch Set 4 : update pubspec/changelog and add some error logging #

Patch Set 5 : update test to use new string formatter so it looks nicer #

Patch Set 6 : update status file #

Patch Set 7 : assign logger #

Total comments: 8

Created: 5 years, 10 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+375 lines, -38 lines)			Patch
M	.gitignore	View		1 chunk	+1 line, -1 line	0 comments	Download
M	.status	View	1 2 3 4 5	1 chunk	+4 lines, -0 lines	0 comments	Download
M	CHANGELOG.md	View	1 2 3	1 chunk	+24 lines, -0 lines	2 comments	Download
A	lib/build/html_import_annotation_inliner.dart	View	1 2 3 4 5 6	1 chunk	+124 lines, -0 lines	4 comments	Download
M	lib/custom_element_proxy.dart	View		1 chunk	+2 lines, -0 lines	0 comments	Download
A	lib/html_import_annotation.dart	View	1	1 chunk	+36 lines, -0 lines	0 comments	Download
A	lib/src/normalize_path.dart	View	1	1 chunk	+28 lines, -0 lines	0 comments	Download
M	pubspec.yaml	View	1 2 3 4	2 chunks	+12 lines, -2 lines	0 comments	Download
A	test/build/html_import_annotation_inliner_test.dart	View	1 2 3 4	1 chunk	+80 lines, -0 lines	0 comments	Download
M	test/custom_element_proxy_test.dart	View	1	1 chunk	+30 lines, -32 lines	0 comments	Download
A	test/html_import_annotation_test.dart	View	1	1 chunk	+28 lines, -0 lines	2 comments	Download
A +	test/html_import_annotation_test.html	View		2 chunks	+5 lines, -3 lines	0 comments	Download
A	test/my_import.html	View		1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 11 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

jakemac

Sending this to you John since Siggi is going to be out :)

5 years, 11 months ago (2015-01-27 23:38:49 UTC) #2

Jennifer Messerly

On 2015/01/27 23:38:49, jakemac wrote: > Sending this to you John since Siggi is going ...

5 years, 10 months ago (2015-01-29 21:23:11 UTC) #3

Jennifer Messerly

this looks like a great start to me. should I take a look at the ...

5 years, 10 months ago (2015-01-29 21:32:38 UTC) #4

jakemac

Replied to everything and updated to use the new ImportPluginTransformer. https://codereview.chromium.org/882823003/diff/1/lib/build/html_import_annotation_inliner.dart File lib/build/html_import_annotation_inliner.dart (right): https://codereview.chromium.org/882823003/diff/1/lib/build/html_import_annotation_inliner.dart#newcode88 ...

5 years, 10 months ago (2015-02-02 20:55:57 UTC) #5

Replied to everything and updated to use the new ImportPluginTransformer.

https://codereview.chromium.org/882823003/diff/1/lib/build/html_import_annota...
File lib/build/html_import_annotation_inliner.dart (right):

https://codereview.chromium.org/882823003/diff/1/lib/build/html_import_annota...
lib/build/html_import_annotation_inliner.dart:88: final RegExp
_htmlImportWithRawString = new RegExp(
On 2015/01/29 21:32:37, John Messerly wrote:
> this is vaguely terrifying :)
> 
> although it gives me an excuse to link the best stackoverflow answer ever:
>
http://stackoverflow.com/questions/1732348/regex-match-open-tags-except-xhtml...
> 
> is there any way we can avoid regular expressions here? e.g. use the parser in
> dartanalyzer? it's really fast if you're just parsing

Done, now using InitializerPluginTransformer which uses the analyzers parser to
do this.

https://codereview.chromium.org/882823003/diff/1/pubspec.yaml
File pubspec.yaml (right):

https://codereview.chromium.org/882823003/diff/1/pubspec.yaml#newcode13
pubspec.yaml:13: html5lib: '>=0.12.0 <0.13.0'
On 2015/01/29 21:32:37, John Messerly wrote:
> fyi, these can be ^0.12.0 now
> (etc)

This would require changing the environment spec I assume? Will save this for a
later cl once I have a chance to evaluate.

https://codereview.chromium.org/882823003/diff/1/pubspec.yaml#newcode22
pubspec.yaml:22: url: git@github.com:dart-lang/initialize.git
On 2015/01/29 21:32:37, John Messerly wrote:
> is the plan to fix this before publishing?

Yes before publishing I will definitely be removing this

https://codereview.chromium.org/882823003/diff/1/test/build/common.dart
File test/build/common.dart (right):

https://codereview.chromium.org/882823003/diff/1/test/build/common.dart#newcode7
test/build/common.dart:7: import
'package:code_transformers/src/test_harness.dart';
On 2015/01/29 21:32:38, John Messerly wrote:
> should code_transformers export this as a public library? or is there a public
> import we can use here?

Heh yes there is, I was copying the pattern from polymer but it looks  like I
can drop this whole file :)

https://codereview.chromium.org/882823003/diff/1/test/build/common.dart#newco...
test/build/common.dart:15: return helper.checkAll(expectedFiles).whenComplete(()
=> helper.tearDown());
On 2015/01/29 21:32:38, John Messerly wrote:
> `() => helper.tearDown()` replace with `helper.tearDown`?

This is gone now

https://codereview.chromium.org/882823003/diff/1/test/build/html_import_annot...
File test/build/html_import_annotation_inliner_test.dart (right):

https://codereview.chromium.org/882823003/diff/1/test/build/html_import_annot...
test/build/html_import_annotation_inliner_test.dart:32: new InitEntry(const
i1.HtmlImport('foo.html'), const LibraryIdentifier(#foo, null, 'web/foo.dart')),
On 2015/01/29 21:32:38, John Messerly wrote:
> run dartfmt  on this file?

Did this on the whole project, these kind of have to stay as single line
statements though.

https://codereview.chromium.org/882823003/diff/1/test/build/html_import_annot...
test/build/html_import_annotation_inliner_test.dart:48: <html><head><link
rel="import" href="foo.html"><link rel="import" href="foo/foo.html"><link
rel="import" href="packages/foo/foo.html"><link rel="import"
href="packages/foo/foo/foo.html"><link rel="import"
href="packages/bar/bar.html"><link rel="import"
href="packages/bar/bar/bar.html"></head><body>
On 2015/01/29 21:32:38, John Messerly wrote:
> long line in string

Unfortunately this is kind of necessary, it reflects the actual
document.outerHtml. I would like to add options to testPhases to remove all
newlines/whitespace when comparing outputs but that doesn't exist yet.

Jennifer Messerly

lgtm https://codereview.chromium.org/882823003/diff/1/pubspec.yaml File pubspec.yaml (right): https://codereview.chromium.org/882823003/diff/1/pubspec.yaml#newcode13 pubspec.yaml:13: html5lib: '>=0.12.0 <0.13.0' On 2015/02/02 20:55:57, jakemac wrote: ...

5 years, 10 months ago (2015-02-02 21:07:51 UTC) #6

jakemac

added some extra error logs and updated the pubspec/changelog https://codereview.chromium.org/882823003/diff/40001/lib/build/html_import_annotation_inliner.dart File lib/build/html_import_annotation_inliner.dart (right): https://codereview.chromium.org/882823003/diff/40001/lib/build/html_import_annotation_inliner.dart#newcode23 lib/build/html_import_annotation_inliner.dart:23: ...

5 years, 10 months ago (2015-02-02 21:53:56 UTC) #7

jakemac

Committed patchset #7 (id:120001) manually as 2aaa0b548c02fd094d3903009546bf15a3984568 (presubmit successful).

5 years, 10 months ago (2015-02-03 15:14:32 UTC) #8

Siggi Cherem (dart-lang)

I'm just catching up with this change - I added a few side comments, but ...

5 years, 10 months ago (2015-02-12 16:49:44 UTC) #10

jakemac

5 years, 10 months ago (2015-02-12 17:04:09 UTC) #11

Message was sent while issue was closed.

https://codereview.chromium.org/882823003/diff/120001/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/882823003/diff/120001/CHANGELOG.md#newcode20
CHANGELOG.md:20: - web_components/build/html_import_annotation_inliner:
On 2015/02/12 16:49:44, Siggi Cherem (dart-lang) wrote:
> maybe we should provide a second transformer:
>  - html_initialize
> 
> that combines what initialize + html_import_annotation_inliner do.
> 
> If a user depends on initialize on their own, they can use this form you
present
> here, but if they don't, they can do everything with a single transformer.
> 
> WDYT?

In the new version of this stuff there will only be a web_components
transformer, which itself calls the initialize transformer (with an html imports
plugin).

https://codereview.chromium.org/882823003/diff/120001/lib/build/html_import_a...
File lib/build/html_import_annotation_inliner.dart (right):

https://codereview.chromium.org/882823003/diff/120001/lib/build/html_import_a...
lib/build/html_import_annotation_inliner.dart:64: ..attributes = {'rel':
'import', 'href': importPath,};
On 2015/02/12 16:49:44, Siggi Cherem (dart-lang) wrote:
> nit: remove trailing comma `,`?

oh I probably had these on separate lines in which case the trailing comma made
sense, and then the formatter removed it, will do

https://codereview.chromium.org/882823003/diff/120001/lib/build/html_import_a...
lib/build/html_import_annotation_inliner.dart:86: // supported for the
transformer.
On 2015/02/12 16:49:44, Siggi Cherem (dart-lang) wrote:
> FYI - I opened https://github.com/dart-lang/initialize/issues/22 based on our
> discussions from earlier this week.

I replied there, but basically in the new plugin architecture this isn't needed.
Plugins have access to a Resolver and can do this on their own as desired.

https://codereview.chromium.org/882823003/diff/120001/test/html_import_annota...
File test/html_import_annotation_test.dart (right):

https://codereview.chromium.org/882823003/diff/120001/test/html_import_annota...
test/html_import_annotation_test.dart:4: @HtmlImport(importPath)
On 2015/02/12 16:49:44, Siggi Cherem (dart-lang) wrote:
> I'm surprised this works in the transformer without using the constant
evaluator
> (as in https://github.com/dart-lang/initialize/issues/22), am I missing
> something? 

This works but will be injected at runtime since the transformer doesn't know
how to deal with it. Again though in the next version its not an issue at all
:).

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages