Skip to content

feat(ivy): ngtsc support for static resolution of array.slice()#27158

Closed
JoostK wants to merge 1 commit intoangular:masterfrom
JoostK:ivy-ngtsc-static-eval-array-slice
Closed

feat(ivy): ngtsc support for static resolution of array.slice()#27158
JoostK wants to merge 1 commit intoangular:masterfrom
JoostK:ivy-ngtsc-static-eval-array-slice

Conversation

@JoostK
Copy link
Member

@JoostK JoostK commented Nov 18, 2018

For ngcc's processing of ES5 bundles, the spread syntax has been
downleveled from [...ARRAY] to become ARRAY.slice(). This commit
adds basic support for static resolution of such call.

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)
  • 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?

ngtsc is unable to process ES5 bundles in which the spread operator is downleveled into an Array.prototype.slice call.

What is the new behavior?

Support for static resolution of such calls.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

For ngcc's processing of ES5 bundles, the spread syntax has been
downleveled from `[...ARRAY]` to become `ARRAY.slice()`. This commit
adds basic support for static resolution of such call.
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Another nice catch!

@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer effort1: hours freq1: low target: major This PR is targeted for the next major release comp: ivy risk: low labels Nov 18, 2018
@petebacondarwin petebacondarwin added this to the Ivy milestone Nov 18, 2018
@ngbot ngbot bot modified the milestones: Ivy, needsTriage Nov 18, 2018
() => { expect(evaluate(`const a = [1, 2, 3];`, 'a[\'length\'] + 1')).toEqual(4); });

it('array `slice` function works', () => {
expect(evaluate(`const a = [1, 2, 3];`, 'a[\'slice\']()')).toEqual([1, 2, 3]);
Copy link
Member

Choose a reason for hiding this comment

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

Why does the test use a['slice'] instead of a.slice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copied from above. I can change it as it isn't really relevant here.

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

SLGTM (still)

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Actually I take that back - this version fails CI: https://travis-ci.org/angular/angular/jobs/457091732#L1288

@JoostK JoostK force-pushed the ivy-ngtsc-static-eval-array-slice branch from 02f0151 to ad91a3d Compare November 19, 2018 21:44
@JoostK
Copy link
Member Author

JoostK commented Nov 19, 2018

@petebacondarwin Ah, interesting. I removed the fixup commit to revert to the original code.

@JoostK
Copy link
Member Author

JoostK commented Dec 3, 2018

@petebacondarwin could you approve again? I dropped the faulty commit that broke stuff, everything is green again :)

@alxhub alxhub dismissed petebacondarwin’s stale review December 3, 2018 18:27

Comments addressed

@alxhub alxhub added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 3, 2018
@alxhub
Copy link
Member

alxhub commented Dec 3, 2018

@alxhub alxhub closed this in 75723d5 Dec 3, 2018
@JoostK JoostK deleted the ivy-ngtsc-static-eval-array-slice branch December 3, 2018 22:55
@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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes effort1: hours freq1: low risk: low target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants