feat(@angular/ssr): move CommonEngine API to /node entry-point
#28278
Conversation
angular-robot
bot
added
detected: breaking change
alan-agius4
added
the
target: major
alan-agius4
added
the
action: review
alan-agius4
force-pushed
the
create-secondary-entry-points-ssr
branch
4 times, most recently
from
27c2ee9 to
e582d38
Compare
| } | ||
|
|
||
| recorder ??= tree.beginUpdate(sourceFile.fileName); | ||
| recorder.insertRight(ssrImport.moduleSpecifier.getEnd() - 1, '/node'); |
There was a problem hiding this comment.
Question: What if the user has imported other symbols from @angular/ssr? I think we would want to create a new @angular/ssr/node import and move CommonEngine there, but leave the rest of the imports as is?
There was a problem hiding this comment.
There are no other exported symbols
There was a problem hiding this comment.
Ah cool, that makes this easier then. If so, do we really care about CommonEngine at all? Should we just look for imports of @angular/ssr and replace them with @angular/ssr/node?
I think an unused import {} from '@angular/ssr'; or import * as ssr from '@angular/ssr'; might fall through the cracks here.
There was a problem hiding this comment.
We need to check that we only migrate when the symbols start with CommonEngine otherwise the migration will not be idempotent.
There was a problem hiding this comment.
IMHO, those 2 cases are rare enough that doesn’t warrant to be included.
There was a problem hiding this comment.
We need to check that we only migrate when the symbols start with
CommonEngineotherwise the migration will not be idempotent.
I don't see why an @angular/ssr -> @angular/ssr/node rename wouldn't be idempotent, but I'll defer to you that's it's necessary.
I agree the import {} and import * as ssr cases probably aren't important enough to go out of our way for.
There was a problem hiding this comment.
Actually one case would be if users start users the new API which is exported from '@angular/ssr' (When it's publicly available. We do not want to migrate those to use @angular/ssr/node.
packages/schematics/angular/migrations/update-ssr-imports/migration.ts
Outdated
Show resolved
Hide resolved
Refactored the `CommonEngine` API import path to remove Node.js dependencies from the `@angular/ssr` main entry-point.
BREAKING CHANGE:
The `CommonEngine` API now needs to be imported from `@angular/ssr/node`.
**Before**
```ts
import { CommonEngine } from '@angular/ssr';
```
**After**
```ts
import { CommonEngine } from '@angular/ssr/node';
```
alan-agius4
force-pushed
the
create-secondary-entry-points-ssr
branch
from
e582d38 to
af4f287
Compare
| } | ||
|
|
||
| recorder ??= tree.beginUpdate(sourceFile.fileName); | ||
| recorder.insertRight(ssrImport.moduleSpecifier.getEnd() - 1, '/node'); |
There was a problem hiding this comment.
We need to check that we only migrate when the symbols start with
CommonEngineotherwise the migration will not be idempotent.
I don't see why an @angular/ssr -> @angular/ssr/node rename wouldn't be idempotent, but I'll defer to you that's it's necessary.
I agree the import {} and import * as ssr cases probably aren't important enough to go out of our way for.
alan-agius4
added
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Refactored the
CommonEngineAPI import path to remove Node.js dependencies from the@angular/ssrmain entry-point.BREAKING CHANGE:
The
CommonEngineAPI now needs to be imported from@angular/ssr/node.Before
After