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

feat: remove deprecated DOCUMENT token from platform-browser #28117

Closed
wants to merge 1 commit into from

Conversation

CaerusKaru
Copy link
Member

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?

DOCUMENT is a token exported by both @angular/platform-browser (deprecated) and @angular/common

Issue Number: N/A

What is the new behavior?

DOCUMENT is no longer a token exported from @angular/platform-browser. It is now only exported from @angular/common

Does this PR introduce a breaking change?

  • Yes
  • No

@CaerusKaru
Copy link
Member Author

cc @IgorMinar

The saucelabs failure is a flake and just needs to be restarted.

@alfaproject
Copy link
Contributor

A refactor doesn't change behaviour or break compatibility. Behaviour is being changed here and there's nothing being fixed, so I guess this is a feat commit.

@CaerusKaru
Copy link
Member Author

@alfaproject Yeah I don't know. It's literally the opposite of a feat though, since it's removing something from the publicApi. And yet, technically, it's removed from the publicApi when it's deprecated. I'll wait until @IgorMinar weighs in, but I have no issue changing the scope to whatever's most appropriate.

@IgorMinar IgorMinar added this to the v8-candidates milestone Jan 14, 2019
@ngbot ngbot bot removed this from the v8-candidates milestone Jan 14, 2019
@IgorMinar
Copy link
Contributor

I expect that this will break a significant amount of code at google. I'm running a presubmit now to verify the impact.

If confirmed, we might need to create a ts-lint refactoring transform to make this change automatically and roll this out as part of ng update for v8.

@StephenFluin can we get a reliable estimate of this change on external users? I'm quite sure it's significant and therefore we could roll this out only with automated refactoring of apps and libraries (which in this case is safe and relatively straightforward).

@IgorMinar
Copy link
Contributor

IgorMinar commented Jan 14, 2019

internal presubmit results: http://test/OCL:229126105:BASE:229126324:1547447183003:cc4ee4a9

preliminary results: there is a handful of apps that still work after this change, but great majority of apps in google3 are broken.

this means that we'll need the ts-lint transform similar to several we built for rxjs in order to roll this change out. @CaerusKaru are you up for taking that on?

Now in order to make the transform compatible with ng update, we'll need a separate package that will contain the rule and the dependency on ts-lint (we can't add a dependency on ts-lint from @angular/platform-browser). So I'm proposing that we create @angular/platform-browser-compat (or possibly a more generic @angular/compat package), similar to rxjs-compat, that will contain the ts-lint transform and this package will declare the dependency on ts-lint.

We then write a ng update schematics for @angular/platform-browser that will install this compat package (and tslint), execute the refactoring, and remove the compat package once successful.

@IgorMinar
Copy link
Contributor

and yes, "refactor" is misleading. we might need a new category for this type of change, but in the meantime use "feat" as weird as it is because it's best category in terms of user visibility of the change in the changelog.

@IgorMinar IgorMinar added this to the v8-candidates milestone Jan 14, 2019
@ngbot ngbot bot removed this from the v8-candidates milestone Jan 14, 2019
@IgorMinar IgorMinar added state: blocked breaking changes area: core Issues related to the framework runtime labels Jan 14, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jan 14, 2019
@IgorMinar IgorMinar added effort2: days risk: high feature Issue that requests a new feature labels Jan 14, 2019
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Jan 14, 2019
@StephenFluin
Copy link
Contributor

Around 80% of apps I just scanned use the DOCUMENT InjectionToken. I can't tell from compiled source where they import it from though, so I have a hard time guessing how many will be affected.

I wouldn't have guessed that it would be many people, as I would guess most developers access document directly and assume web-only use cases.

@CaerusKaru
Copy link
Member Author

Blocked on #29237

@CaerusKaru
Copy link
Member Author

Also blocked on angular/components#15470

@CaerusKaru
Copy link
Member Author

bump @alexeagle @IgorMinar

This can be merged now that #29950 has been merged

@mhevery mhevery self-assigned this Apr 23, 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 area: core Issues related to the framework runtime breaking changes cla: yes effort2: days feature Issue that requests a new feature risk: high 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

8 participants