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

Deprecate withServerTransition call and generate static APP_ID #49422

Closed
wants to merge 3 commits into from

Conversation

alan-agius4
Copy link
Contributor

@alan-agius4 alan-agius4 commented Mar 14, 2023

feat(platform-browser): deprecate withServerTransition call
This commit deprecated BrowserModule.withServerTransition instead APP_ID should be used instead to configure the app id.

DEPRECATED: BrowserModule.withServerTransition has been deprecated. APP_ID should be used instead to set the application ID.
NB: Unless, you render multiple Angular applications on the same page, setting an application ID is not necessary.

Before:

imports: [
  BrowserModule.withServerTransition({ appId: 'serverApp' }),
  ...
]

After:

imports: [
  BrowserModule,
  { provide: APP_ID, useValue: 'serverApp' },
  ...
],

refactor(core): generate a static application ID.
Prior to this change, a random application ID was generated each time which forced users using server rendering to provide an application ID themselves. This was needed to handle rare cases when multiple Angular applications are rendered on the same page.

With this change the application ID is no longer generated randomly and instead it is hard coded.

BREAKING CHANGE:

The APP_ID token value is no longer randomly generated. If you are bootstrapping multiple application on the same page you will need to set to provide the APP_ID yourself.

bootstrapApplication(ComponentA, {
  providers: [
   { provide: APP_ID, useValue: 'app-a' },
   // ... other providers ...
  ]
});

@alan-agius4 alan-agius4 added the target: major This PR is targeted for the next major release label Mar 14, 2023
@angular-robot angular-robot bot added detected: deprecation PR contains a commit with a deprecation detected: feature PR contains a feature commit labels Mar 14, 2023
@alan-agius4 alan-agius4 force-pushed the app_id_deprecate branch 2 times, most recently from bf94712 to 30edba2 Compare March 14, 2023 11:56
@alan-agius4 alan-agius4 marked this pull request as ready for review March 14, 2023 12:39
@alan-agius4 alan-agius4 added action: review The PR is still awaiting reviews from at least one requested reviewer area: server Issues related to server-side rendering labels Mar 14, 2023
@ngbot ngbot bot added this to the Backlog milestone Mar 14, 2023
@alan-agius4 alan-agius4 added the area: core Issues related to the framework runtime label Mar 14, 2023
@JeanMeche
Copy link
Member

Nit: You could add an entry to the depreciation guide.

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

packages/core/src/application_tokens.ts Outdated Show resolved Hide resolved
*
* @publicApi
*/
export const APP_ID = new InjectionToken<string>('AppId', {
providedIn: 'root',
factory: _appIdRandomProviderFactory,
factory: () => 'ng',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we can extract this value to a separate const to make it more self-documenting:

Suggested change
factory: () => 'ng',
factory: () => DEFAULT_APP_ID,

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's just static, can we use a value provider? useValue: DEFAULT_APP_ID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, the default value, will be set in a totally different package since an InjectorToken doesn’t allow useValue.

Ex:

{provide: INJECTOR_SCOPE, useValue: 'root'},

@@ -49,12 +49,6 @@ function _render<T>(
bootstrapPromise: Promise<NgModuleRef<T>|ApplicationRef>): Promise<string> {
return bootstrapPromise.then((moduleOrApplicationRef) => {
const environmentInjector = moduleOrApplicationRef.injector;
const transitionId = environmentInjector.get(APP_ID, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also remove this call (in the same file)?

importProvidersFrom(BrowserModule.withServerTransition({appId})),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not remove this one as this overload of renderApplication will be deleted in a follow up PR.

Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

@pullapprove pullapprove bot requested a review from atscott March 14, 2023 19:09
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

Is the second commit (refactor(core): generate a static application ID.) a breaking change for when there are multiple applications (however rare that would be)

@pullapprove pullapprove bot requested a review from atscott March 14, 2023 19:16
@alan-agius4
Copy link
Contributor Author

Is the second commit (refactor(core): generate a static application ID.) a breaking change for when there are multiple applications (however rare that would be)

I’ll add a breaking change message.

This commit deprecated ` BrowserModule.withServerTransition` instead `APP_ID` should be used instead to configure the app id.

DEPRECATED: `BrowserModule.withServerTransition` has been deprecated. `APP_ID` should be used instead to set the application ID.
NB: Unless, you render multiple Angular applications on the same page, setting an application ID is not necessary.

Before:
```ts
imports: [
  BrowserModule.withServerTransition({ appId: 'serverApp' }),
  ...
]
```

After:
```ts
imports: [
  BrowserModule,
  { provide: APP_ID, useValue: 'serverApp' },
  ...
],
```
Prior to this change, a random application ID was generated each time which forced users using server rendering to provide an application ID themselves. This was needed to handle rare cases when multiple Angular applications are rendered on the same page.

With this change the application ID is no longer generated randomly and instead it is hard coded.

BREAKING CHANGE:

The `APP_ID` token value is no longer randomly generated. If you are bootstrapping multiple application on the same page you will need to set to provide the `APP_ID` yourself.

```ts
bootstrapApplication(ComponentA, {
  providers: [
   { provide: APP_ID, useValue: 'app-a' },
   // ... other providers ...
  ]
});
```
This commit removes the usages of `withServerTransition` form tests.
@angular-robot angular-robot bot added the detected: breaking change PR contains a commit with a breaking change label Mar 15, 2023
@alan-agius4
Copy link
Contributor Author

@atscott, PTAL.

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@alan-agius4 alan-agius4 removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Mar 15, 2023
@alan-agius4 alan-agius4 added the action: merge The PR is ready for merge by the caretaker label Mar 15, 2023
@alxhub
Copy link
Member

alxhub commented Mar 16, 2023

This PR was merged into the repository by commit cf98777.

@alxhub alxhub closed this in 630af63 Mar 16, 2023
alxhub pushed a commit that referenced this pull request Mar 16, 2023
Prior to this change, a random application ID was generated each time which forced users using server rendering to provide an application ID themselves. This was needed to handle rare cases when multiple Angular applications are rendered on the same page.

With this change the application ID is no longer generated randomly and instead it is hard coded.

BREAKING CHANGE:

The `APP_ID` token value is no longer randomly generated. If you are bootstrapping multiple application on the same page you will need to set to provide the `APP_ID` yourself.

```ts
bootstrapApplication(ComponentA, {
  providers: [
   { provide: APP_ID, useValue: 'app-a' },
   // ... other providers ...
  ]
});
```

PR Close #49422
alxhub pushed a commit that referenced this pull request Mar 16, 2023
This commit removes the usages of `withServerTransition` form tests.

PR Close #49422
@alan-agius4 alan-agius4 deleted the app_id_deprecate branch March 16, 2023 05:59
alan-agius4 added a commit to alan-agius4/angular-cli that referenced this pull request Mar 24, 2023
…rverTransition`

 This commit removes generation of `.withServerTransition` in the universal schematic as is deprecated.

 DEPRECATED: the `app-id` option in the app-shell and universal schematics has been deprecated without replacement. See: angular/angular#49422 for more information about the rational of this change.
alan-agius4 added a commit to alan-agius4/angular-cli that referenced this pull request Mar 24, 2023
…rverTransition`

 This commit removes generation of `.withServerTransition` in the universal schematic as is deprecated.

 DEPRECATED: the `app-id` option in the app-shell and universal schematics has been deprecated without replacement. See: angular/angular#49422 for more information about the rational of this change.
angular-robot bot pushed a commit to angular/angular-cli that referenced this pull request Mar 24, 2023
…rverTransition`

 This commit removes generation of `.withServerTransition` in the universal schematic as is deprecated.

 DEPRECATED: the `app-id` option in the app-shell and universal schematics has been deprecated without replacement. See: angular/angular#49422 for more information about the rational of this change.
@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 Apr 16, 2023
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 area: server Issues related to server-side rendering detected: breaking change PR contains a commit with a breaking change detected: deprecation PR contains a commit with a deprecation detected: feature PR contains a feature commit 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

6 participants