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.

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

refactor(ivy): migrate ɵɵ prefix back to Δ #30362

Closed
wants to merge 3 commits into from

Conversation

benlesh
Copy link
Contributor

@benlesh benlesh commented May 9, 2019

Now that issues are resolved with Closure compiler, we can move back to our desired prefix of Δ.

@benlesh benlesh added comp: ivy target: major This PR is targeted for the next major release risk: medium refactoring Issue that involves refactoring or code-cleanup labels May 9, 2019
@ngbot ngbot bot modified the milestone: needsTriage May 9, 2019
@benlesh benlesh force-pushed the back-to-delta branch 5 times, most recently from 69b0dac to 8d95ba8 Compare May 13, 2019 23:33
@benlesh benlesh marked this pull request as ready for review May 14, 2019 06:17
@benlesh benlesh requested review from IgorMinar and a team as code owners May 14, 2019 06:17
@benlesh
Copy link
Contributor Author

benlesh commented May 14, 2019

presubmit

@benlesh benlesh added the action: merge The PR is ready for merge by the caretaker label May 14, 2019
@ngbot
Copy link

ngbot bot commented May 14, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "angular" is failing
    pending status "ci/angular: size" is pending
    pending status "ci/circleci: integration_test" is pending
    pending status "ci/circleci: material-unit-tests" is pending
    pending status "ci/circleci: test" is pending
    pending status "ci/circleci: test_aio" is pending
    pending status "ci/circleci: test_aio_local" is pending
    pending status "ci/circleci: test_aio_local_ivy" is pending
    pending status "ci/circleci: test_docs_examples" is pending
    pending status "ci/circleci: test_docs_examples_ivy" is pending
    pending status "ci/circleci: test_ivy_aot" is pending
    pending status "google3" is pending
    pending missing required status "ci/circleci: publish_snapshot"
    pending 1 pending code review

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@gkalpak
Copy link
Member

gkalpak commented May 14, 2019

The reason the test_aio_local/test_aio_local_ivy jobs (which use the locally built Angular packages) are failing is Material components are trying to import things like ɵɵdefineInjectable and ɵɵinject from @angular/core, which no longer exist (for example see material/esm2015/icons.js).

There are warning about this in the yarn build-local step on the CircleCI job.
No idea why these are just warnings and not hard failures 😕 (It does fail at runtime.)

So, if there are published packages that use ɵɵ-prefixed exports, what is the plan for them with the switch to Δ?
Or is it just material?

UPDATE: Filed angular/angular-cli#14421 to investigate why no hard failures.

@ocombe
Copy link
Contributor

ocombe commented May 14, 2019

Why is material using private APIs? The point of these being private is that we should be able to change them without any issue

@devversion
Copy link
Member

devversion commented May 14, 2019

@ocombe Material is not using these directly. Since Angular Material uses tree-shakeable injectables which were introduced in V6 (see here), NGC generates ngInjectableDef static class members which rely on the defineInjectable function.

See for example: https://github.com/angular/material2-builds/blob/master/bundles/material-core.umd.js

Ideally the defineInjectable public symbol would have never been prefixed/updated in such a way as it breaks backwards compatibility.

@benlesh
Copy link
Contributor Author

benlesh commented May 14, 2019

Ugh... I think they were supposed to use defineInjectable which is marked as a @deprecated @publicApi and not @codeGenApi like ɵɵdefineInjectable. 🤦‍♂️

Not sure what to do here. I don't think we want TWO deprecated versions of defineInjectable.

@mhevery?

@gkalpak
Copy link
Member

gkalpak commented May 14, 2019

Who is "they"? According to #30362 (comment), this is ngc's doing (which would imply other 3rd party libraries are affected?).

@devoto13
Copy link
Contributor

devoto13 commented May 14, 2019

I'm currently updating a library to compile it with Angular 8 and I believe I face a related issue. ngc from Angular 6 and 7 was emitting:

FaIconService.ngInjectableDef = i0.defineInjectable({ factory: function FaIconService_Factory() { return new FaIconService(); }, token: FaIconService, providedIn: "root" });

While from Angular 8 it emits:

FaIconService.ngInjectableDef = i0.ɵɵdefineInjectable({ factory: function FaIconService_Factory() { return new FaIconService(); }, token: FaIconService, providedIn: "root" });

As far as I can see updating library to Angular 8 forces it to drop support for all previous Angular versions as they don't have ɵɵdefineInjectable function. Just want to double check that this change was intentional?

@benlesh
Copy link
Contributor Author

benlesh commented May 14, 2019

Okay... so I'm just deprecating ɵɵdefineInjectable and ɵɵinject until we're out of RC, at which point it will be deleted.

@benlesh
Copy link
Contributor Author

benlesh commented May 14, 2019

@devoto13 The angular team is dedicated to backward compatibility. This is probably just a growing pain with Ivy and the RC. Please file an issue to let us know what problems your experiencing, ideally with a repro. Then you can DM it to me and I'll try to get it escalated so we discuss it. We definitely don't want to break anyone :)

(To DM me, the best bet is twitter, my GitHub notifications are a trainwreck)

@alxhub alxhub added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label May 14, 2019
Now that issues are resolved with Closure compiler, we can move back to our desired prefix of `Δ`.
@benlesh benlesh 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 May 14, 2019
@benlesh
Copy link
Contributor Author

benlesh commented May 14, 2019

newer presubmit

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

LGTM but please write more descriptive commit messages explaining why we are making this change at all.

@IgorMinar
Copy link
Contributor

note from multiple 1:1 discussions with @benlesh, @alxhub, @mhevery and @kara: @benlesh is to create a new PR targeting the 8.0.x branch so that we can keep on cherry-picking fixes and infrastructure improvements between the two branches easily.

- They are to be removed before the end of RC
@alxhub alxhub closed this in cf86ed7 May 14, 2019
alxhub pushed a commit that referenced this pull request May 14, 2019
alxhub pushed a commit that referenced this pull request May 14, 2019
- They are to be removed before the end of RC

PR Close #30362
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
Now that issues are resolved with Closure compiler, we can move back to our desired prefix of `Δ`.

PR Close angular#30362
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
@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 15, 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 refactoring Issue that involves refactoring or code-cleanup risk: medium target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants