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

Tslib as peerDependency #32167

Closed
wants to merge 1 commit into from
Closed

Tslib as peerDependency #32167

wants to merge 1 commit into from

Conversation

alan-agius4
Copy link
Contributor

@alan-agius4 alan-agius4 commented Aug 16, 2019

feat: change tslib from direct dependency to peerDependency

BREAKING CHANGE:

We no longer directly have a direct depedency on tslib. Instead it is now listed a peerDependency.

Users not using the CLI will need to manually install tslib via;

yarn add tslib

or

npm install tslib --save

Reference: TOOL-836

@alan-agius4 alan-agius4 requested review from a team as code owners August 16, 2019 14:44
@alan-agius4 alan-agius4 added target: major This PR is targeted for the next major release action: review The PR is still awaiting reviews from at least one requested reviewer PR action: time-zone area: bazel Issues related to the published `@angular/bazel` build rules labels Aug 16, 2019
@ngbot ngbot bot added this to the needsTriage milestone Aug 16, 2019
@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Aug 16, 2019

@alan-agius4 alan-agius4 requested review from a team as code owners August 16, 2019 15:19
@alan-agius4 alan-agius4 added state: WIP and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 16, 2019
@alan-agius4
Copy link
Contributor Author

Will look into the failures over the weekend.

@gkalpak, can you give me a tip we I should be looking for the docs failures? Thanks

@alan-agius4
Copy link
Contributor Author

CI is green.

@alan-agius4 alan-agius4 added action: review The PR is still awaiting reviews from at least one requested reviewer breaking changes labels Aug 17, 2019
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.

Why this change? I have two concerns:

  • umd bundles are now no longer self-contained, breaking universal and anyone relying on umd
  • the change from dep to peerDep requires a migration, but none is attached in this change

Can we please first discuss the motivation behind this and then we can decide if we want to go forward or not? thanks

@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Aug 26, 2019

Hi @IgorMinar, thanks for taking a look.

umd bundles are now no longer self-contained, breaking universal and anyone relying on umd

No fully understanding why this breaks universal apart from when not having tslib as a dev dependency,

I do agree that UMD bundles are not "self-contained" to an extend since RxJs is not embedded either. But yes, it will break users using UMD's such as system.js users.

the change from dep to peerDep requires a migration, but none is attached in this change

Indeed a migration needs to be added, though that should be trivial to that and most of the CLI users are already using tslib. As it's already added to devDepedencies since importHelpers is enabled by default.

Can we please first discuss the motivation behind this and then we can decide if we want to go forward or not?

The main motivation for this is that tslib is included entirely in all the UMD bundles.
For example, if a users adds 4 UMDs bundles in their application each of these 4 bundles will contain a full version of tslib.

I am not sure if @alexeagle had some other reasons for this, apart from what I mentioned, since he originally opened the issue (TOOL-836)

Though, I'd like an answer to a question. Why a dependency on tslib is different then let's say RxJs which is always a peerDepedency, it is because it's a small dependency (approx 12Kb unminified) or there are other motivations?

Thanks

@alexeagle
Copy link
Contributor

I'm pretty sure the request came from @IgorMinar originally, as a way to trim bundle sizes.

@petebacondarwin
Copy link
Member

@IgorMinar what are we going to do about this PR?

@petebacondarwin
Copy link
Member

We should get approval from @IgorMinar and then this will need a new rebase @alan-agius4

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.

summarizing the in person discussion:

  • let's change the PR to keep on inlining the tslib into umd bundles (umd is used on CDNs and we don't have way to not making this change a breaking one, which would require everyone who depends on umds update their index.html or whatever module loader / resolver they use.
  • let's include a migration that ensure that all cli projects have tslib installed as a dependency.
  • let's include this migration in the list of @kara's migrations for v9

otherwise lgtm

@alan-agius4
Copy link
Contributor Author

Migration to ensure that tslib is installed angular/angular-cli#15800

BREAKING CHANGE:

We no longer directly have a direct depedency on `tslib`. Instead it is now listed a `peerDependency`.

Users not using the CLI will need to manually install `tslib` via;
```
yarn add tslib
```
or
```
npm install tslib --save
```
@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 14, 2019
@alan-agius4
Copy link
Contributor Author

@IgorMinar, I marked this PR for merge, I added the migration to @kara's list and added a migration to ensure tslib is installed. angular/angular-cli#15800

I also rolled out the changes to components and universal
angular/universal#1278
angular/components#17393

@alan-agius4 alan-agius4 added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Oct 14, 2019
@alan-agius4
Copy link
Contributor Author

alan-agius4 commented Oct 14, 2019

Caretaker: @IgorMinar approved!

@mhevery mhevery closed this in e2d5bc2 Oct 14, 2019
@alan-agius4 alan-agius4 deleted the tslib-peer-deps branch October 14, 2019 16:37
CaerusKaru pushed a commit to angular/universal that referenced this pull request Oct 15, 2019
nguniversal relied on a hidden dependency of `tslib` due to `importHelpers`
https://github.com/angular/universal/blob/5798f198db1fb0bce9dbfb850751429a3724084a/modules/bazel-tsconfig-build.json#L11 which was previously installed by the Angular framework. This, however changed with this PR angular/angular#32167

With this change we now list `tslib` as a required peerDepedency.

Users not using the CLI will need to manually install `tslib` via;
```
yarn add tslib
```
or
```
npm install tslib --save
```

Reference: TOOL-836
ODAVING pushed a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
…32167)

BREAKING CHANGE:

We no longer directly have a direct depedency on `tslib`. Instead it is now listed a `peerDependency`.

Users not using the CLI will need to manually install `tslib` via;
```
yarn add tslib
```
or
```
npm install tslib --save
```

PR Close angular#32167
vikerman pushed a commit to angular/angular-cli that referenced this pull request Oct 22, 2019
AndrusGerman pushed a commit to AndrusGerman/angular that referenced this pull request Oct 22, 2019
…32167)

BREAKING CHANGE:

We no longer directly have a direct depedency on `tslib`. Instead it is now listed a `peerDependency`.

Users not using the CLI will need to manually install `tslib` via;
```
yarn add tslib
```
or
```
npm install tslib --save
```

PR Close angular#32167
@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 Nov 14, 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: bazel Issues related to the published `@angular/bazel` build rules breaking changes cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note 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

5 participants