Skip to content
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

refactor(ivy): add element instruction, reducing output size #24379

Closed
wants to merge 4 commits into from
Closed

refactor(ivy): add element instruction, reducing output size #24379

wants to merge 4 commits into from

Conversation

benlesh
Copy link
Contributor

@benlesh benlesh commented Jun 8, 2018

  • Adds an element instruction
  • Reduces size of compiled output slightly

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

Currently the compiler outputs an elementStart and elementEnd command for every element, even if they self-closed (<input /> for example)

What is the new behavior?

Will output a single element command for self-closing tags

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Factored out of #23899

@benlesh benlesh requested a review from vicb June 8, 2018 18:32
@benlesh benlesh added comp: ivy target: major This PR is targeted for the next major release labels Jun 8, 2018
@mary-poppins
Copy link

You can preview bea02e7 at https://pr24379-bea02e7.ngbuilds.io/.

vicb
vicb reviewed Jun 8, 2018
View reviewed changes
packages/compiler/src/render3/view/template.ts Outdated
@@ -358,6 +366,7 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
}
});


Copy link
Contributor

Choose a reason for hiding this comment

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

nit: revert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

vicb
vicb reviewed Jun 8, 2018
View reviewed changes
packages/compiler/test/render3/r3_compiler_compliance_spec.ts
@@ -90,8 +90,7 @@ describe('compiler compliance', () => {
const template = `
template: function MyComponent_Template(rf: IDENT, ctx: IDENT) {
if (rf & 1) {
$r3$.ɵE(0, 'div');
Copy link
Contributor

Choose a reason for hiding this comment

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

Those compliance tests mirror tests in angular/packages/core/test/render3/compiler_canonical - those ones should be updated as well.

You might want to update all tests in angular/packages/core/test/render3 as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@mary-poppins
Copy link

You can preview 54cdbef at https://pr24379-54cdbef.ngbuilds.io/.

@benlesh benlesh added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jun 8, 2018
@benlesh
Copy link
Contributor Author

benlesh commented Jun 8, 2018

Caretaker: this PR has golden file changes.

@ngbot
Copy link

ngbot bot commented Jun 8, 2018

Hi @benlesh! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@benlesh
Copy link
Contributor Author

benlesh commented Jun 9, 2018

rebased

@mary-poppins
Copy link

You can preview c7d5b9e at https://pr24379-c7d5b9e.ngbuilds.io/.

@mhevery mhevery added the action: merge The PR is ready for merge by the caretaker label Jun 11, 2018
@mhevery
Copy link
Contributor

mhevery commented Jun 11, 2018

http://test/OCL:200062300:BASE:200062325:1528734303782:5faf8dde

@mhevery mhevery removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jun 11, 2018
@mhevery mhevery closed this in 8dd99ac Jun 11, 2018
@mhevery mhevery mentioned this pull request Jun 14, 2018
Closed
3 tasks
@benlesh benlesh deleted the ivy-element-instruction branch July 31, 2018 20:37
@angular-automatic-lock-bot
Copy link

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 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@vicb vicb vicb approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@benlesh @mary-poppins @mhevery @vicb @googlebot