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

commented Jan 13, 2019

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 CaerusKaru requested review from angular/fw-compiler as code owners Jan 13, 2019

@googlebot googlebot added the cla: yes label Jan 13, 2019

@CaerusKaru CaerusKaru force-pushed the CaerusKaru:adam/document branch 2 times, most recently from 3773319 to c8b85a1 Jan 13, 2019

@CaerusKaru CaerusKaru requested a review from angular/fw-integration as a code owner Jan 13, 2019

@CaerusKaru CaerusKaru force-pushed the CaerusKaru:adam/document branch from c8b85a1 to 2e106f1 Jan 13, 2019

@CaerusKaru

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2019

cc @IgorMinar

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

@alfaproject

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2019

@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

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

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

@ngbot ngbot bot added this to the needsTriage milestone Jan 14, 2019

@ngbot ngbot bot modified the milestones: needsTriage, Backlog Jan 14, 2019

@IgorMinar IgorMinar modified the milestones: Backlog, v8-candidates Jan 14, 2019

@CaerusKaru CaerusKaru force-pushed the CaerusKaru:adam/document branch from 2e106f1 to 48c338a Jan 14, 2019

@CaerusKaru CaerusKaru changed the title refactor: remove deprecated DOCUMENT token from platform-browser feat: remove deprecated DOCUMENT token from platform-browser Jan 14, 2019

@StephenFluin

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

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 CaerusKaru force-pushed the CaerusKaru:adam/document branch from 48c338a to 60d1f79 Mar 14, 2019

@CaerusKaru

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

Blocked on #29237

@CaerusKaru

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

Also blocked on angular/components#15470

@CaerusKaru CaerusKaru force-pushed the CaerusKaru:adam/document branch from 60d1f79 to 05b02a9 Mar 16, 2019

@CaerusKaru CaerusKaru force-pushed the CaerusKaru:adam/document branch from 05b02a9 to 6436cdb Apr 17, 2019

@CaerusKaru CaerusKaru force-pushed the CaerusKaru:adam/document branch from 6436cdb to 41e1218 Apr 18, 2019

@CaerusKaru

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

bump @alexeagle @IgorMinar

This can be merged now that #29950 has been merged

@mhevery mhevery self-assigned this Apr 23, 2019

@CaerusKaru CaerusKaru force-pushed the CaerusKaru:adam/document branch from 41e1218 to 30ea564 Apr 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.