Skip to content

refactor(Compiler): inline styles before compiling the template #2563

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
vicb wants to merge 8 commits into from
Closed

refactor(Compiler): inline styles before compiling the template #2563

vicb wants to merge 8 commits into from

Conversation

@vicb
Copy link
Contributor

@vicb vicb commented Jun 16, 2015

TODO:

  • add ref to 1694 to the commit message - fixed in a previous PR
  • implement relative url resolving to fix JS
  • find a way to support querySelectorAll in Dart
  • address TODOs (1 xited tests)

tbosch
tbosch reviewed Jun 16, 2015
View reviewed changes
modules/angular2/src/render/dom/compiler/compiler.ts Outdated
Copy link
Contributor

@tbosch tbosch Jun 16, 2015

Choose a reason for hiding this comment

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

What is the blocker here?

Copy link
Contributor Author

@vicb vicb Jun 16, 2015

Choose a reason for hiding this comment

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

no blocker, I'm on it

@tbosch
Copy link
Contributor

tbosch commented Jun 16, 2015

@vicb This looks good! Is it still WIP or besides the Travis errors good to go?

@vicb vicb changed the title refactor(Compiler): inline styles before compiling the template [WIP] refactor(Compiler): inline styles before compiling the template Jun 16, 2015
@vicb vicb force-pushed the 0615-cssUrls branch 7 times, most recently from d8e297e to 735fd79 Compare June 16, 2015 17:16
@vicb vicb mentioned this pull request Jun 16, 2015
Closed
@vicb
Copy link
Contributor Author

vicb commented Jun 16, 2015

@tbosch I have currently to issues with this PR:
(/ref https://travis-ci.org/angular/angular/builds/67068776)

in Dart

We use the Html5LibDomAdapter adapter and it does not support querySelectorAll which is needed to find <style> tags in the template.

Any idea or an idea on who can help ?

in JS

Because relative url resolution is not (yet) working, we fail to resolve css urls (in material examples). It throws an exception and tests fail.

The best for JS is probably to wait for the resolution to be implemented - I'll start working on this tomorrow.

@tbosch
Copy link
Contributor

tbosch commented Jun 17, 2015

@vicb: Could you walk the template manually, instead of using querySelectorAll?

Regarding JS: How did it work before? I.e. we were able to resolve the url for the html file correctly... Don't see the connection to relative url resolution...

@vicb
Copy link
Contributor Author

vicb commented Jun 17, 2015

@tbosch

  • for JS we used not to inline @import rules in emulated unscoped which explain the current issue,
  • for Dart I would prefer not to reinvent the wheel.

@vicb vicb mentioned this pull request Jun 17, 2015
Closed
@vicb
Copy link
Contributor Author

vicb commented Jun 17, 2015

Blocked on the Dart issue describe in the comments above, any help would be appreciated

@vicb vicb mentioned this pull request Jun 17, 2015
Closed
@vicb
Copy link
Contributor Author

vicb commented Jun 18, 2015

I got some hints from @yjbanov (via @tbosch) to help unblocking this issue, I'll try to implement it next week.

@vicb vicb mentioned this pull request Jun 18, 2015
Closed
@vicb vicb force-pushed the 0615-cssUrls branch 4 times, most recently from 11676c2 to 481eda0 Compare June 18, 2015 10:01
@vicb vicb force-pushed the 0615-cssUrls branch 2 times, most recently from 8219c56 to 07d247f Compare June 24, 2015 07:04
@vicb vicb changed the title [WIP] refactor(Compiler): inline styles before compiling the template refactor(Compiler): inline styles before compiling the template Jun 24, 2015
@vicb vicb added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Jun 24, 2015
@vicb
Copy link
Contributor Author

vicb commented Jun 24, 2015

@tbosch could you please review the PR ?

Highlights:

  • UrlResolver is no more relying on a anchor element. It can now be used in any env (ie server, WebWorkers),
  • Styles get inlined before the template is actually compiled,
  • The JS server DOM adapter has been fixed so that it doesn't try to insert empty text nodes,
  • Last commits are all bout cleanup

I have kept the changes in separate commits in order to make the review less painful.

@tbosch
Copy link
Contributor

tbosch commented Jun 24, 2015

Do you know how and whether the transformer also inlines styles / templates? Do they use our TemplateLoader as well for this?

@tbosch
Copy link
Contributor

tbosch commented Jun 24, 2015

LGTM.
Could you create a new issue for replacing your UrlResolver with a third party library and please ping Igor and Brian on that issue as well for their url parsing needs.

@tbosch tbosch added pr_state: LGTM action: merge The PR is ready for merge by the caretaker action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 24, 2015
@vicb
Copy link
Contributor Author

vicb commented Jun 24, 2015

Do you know how and whether the transformer also inlines styles / templates? Do they use our TemplateLoader as well for this?

I think it has been deferred, see #2599 (comment) (2 new issues have been created to track that)

@vicb vicb removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jun 24, 2015
@vicb
Copy link
Contributor Author

vicb commented Jun 24, 2015

landed as a0e0f31 & previous commits.

Thanks for the review @tbosch

@vicb vicb closed this Jun 24, 2015
@vicb vicb deleted the 0615-cssUrls branch June 24, 2015 16:41
@vicb vicb mentioned this pull request Jun 24, 2015
Closed
2 tasks
@yjbanov
Copy link
Contributor

yjbanov commented Jun 30, 2015

Just a reminder that renames should be marked as "BREAKING CHANGES" so our changelog could pick them up.

@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Sep 6, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@tbosch tbosch

Labels

action: merge The PR is ready for merge by the caretaker cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vicb @tbosch @yjbanov @googlebot