Skip to content

ngcc bundle PR including implementation work for other formats & integration test#25406

Closed
petebacondarwin wants to merge 24 commits into
masterfrom
ngcc-integration-test
Closed

ngcc bundle PR including implementation work for other formats & integration test#25406
petebacondarwin wants to merge 24 commits into
masterfrom
ngcc-integration-test

Conversation

@petebacondarwin
Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin commented Aug 9, 2018

This sits on top of #25203

@petebacondarwin petebacondarwin added feature Label used to distinguish feature request from other issues action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Aug 9, 2018
@petebacondarwin petebacondarwin added this to the Ivy milestone Aug 9, 2018
@googlebot
Copy link
Copy Markdown

googlebot commented Aug 9, 2018

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@alxhub alxhub changed the title feat(ivy): Ngcc integration test and supporting functionality ngcc bundle PR including implementation work for other formats & integration test Aug 9, 2018
@alxhub alxhub force-pushed the ngcc-integration-test branch from 90aa611 to efe310f Compare August 9, 2018 15:00
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Aug 9, 2018

You can preview efe310f at https://pr25406-efe310f.ngbuilds.io/.

JoostK
JoostK reviewed Aug 9, 2018
View reviewed changes
Comment thread packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts Outdated
import {ClassMemberKind, ReflectionHost} from '../../host';

const TS_DTS_EXTENSION = /(\.d)?\.ts$/;
const TS_DTS_JS_EXTENSION = /(\.d)?\.(ts|js)$/;
Copy link
Copy Markdown
Member

@JoostK JoostK Aug 9, 2018

Choose a reason for hiding this comment

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

@alxhub For a file ending in .d.js, all of .d.ts will be removed below, whereas I'd expect .d to be kept.

Copy link
Copy Markdown
Contributor Author

@petebacondarwin petebacondarwin Aug 9, 2018

Choose a reason for hiding this comment

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

Good catch @JoostK .

I think we should go with /((\.d)?\.ts|js)$/

Copy link
Copy Markdown
Member

@JoostK JoostK Aug 9, 2018

Choose a reason for hiding this comment

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

@petebacondarwin actually: /((\.d)?\.ts|\.js)$/

Copy link
Copy Markdown
Contributor Author

@petebacondarwin petebacondarwin Aug 9, 2018

Choose a reason for hiding this comment

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

Oops. Yes :-)

@alxhub alxhub force-pushed the ngcc-integration-test branch from efe310f to 99b2e7e Compare August 9, 2018 17:58
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Aug 9, 2018

You can preview 99b2e7e at https://pr25406-99b2e7e.ngbuilds.io/.

@alxhub alxhub force-pushed the ngcc-integration-test branch from 99b2e7e to 82c8052 Compare August 10, 2018 12:36
@mary-poppins
Copy link
Copy Markdown

mary-poppins commented Aug 10, 2018

You can preview 82c8052 at https://pr25406-82c8052.ngbuilds.io/.

@alxhub alxhub added cla: yes and removed cla: no labels Aug 10, 2018
@googlebot
Copy link
Copy Markdown

googlebot commented Aug 10, 2018

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@alxhub
Copy link
Copy Markdown
Member

alxhub commented Aug 10, 2018

http://test/OCL:208201706:BASE:208201747:1533908396120:7553c6e7

@petebacondarwin petebacondarwin mentioned this pull request Aug 12, 2018
Closed
@googlebot
Copy link
Copy Markdown

googlebot commented Aug 16, 2018

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

This was referenced Aug 17, 2018
Closed
Closed
Closed
matsko pushed a commit that referenced this pull request Aug 22, 2018
@petebacondarwin @matsko
feat(ivy): find all packages to be compiled by ngcc (#25406)
7500f0e 
PR Close #25406
matsko pushed a commit that referenced this pull request Aug 22, 2018
@alxhub @matsko
fix(ivy): ensure factory statements are emitted correctly (#25406)
61218f5 
PR Close #25406
@IgorMinar IgorMinar deleted the ngcc-integration-test branch October 16, 2018 21:36
@JoostK JoostK mentioned this pull request Nov 12, 2018
Closed
14 tasks
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot Bot commented Sep 14, 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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JoostK JoostK JoostK left review comments

@alxhub alxhub alxhub approved these changes

+2 more reviewers

@gkalpak gkalpak gkalpak approved these changes

@mhevery mhevery mhevery approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker cla: yes feature Label used to distinguish feature request from other issues merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release

Projects

None yet

Milestone

Ivy (not in use)

Development

Successfully merging this pull request may close these issues.

7 participants

@petebacondarwin @googlebot @mary-poppins @alxhub @gkalpak @mhevery @JoostK