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

Jump to bottom

@angular/localize should have @babel/core as a devDependency #38329

Closed
sorahn opened this issue Aug 3, 2020 · 6 comments
Closed

@angular/localize should have @babel/core as a devDependency #38329

sorahn opened this issue Aug 3, 2020 · 6 comments
Labels
area: i18n type: use-case
Milestone
Backlog

Comments

@sorahn
Copy link

sorahn commented Aug 3, 2020

🐞 bug report

Affected Package

@angular/localize

Is this a regression?

not sure

Description

@angular/localize has @babel/core as a dependency, and not a devDependency.

We use BlackDuck for all the license attributions for the code we ship, but because @babel/core is include here, all the @babel/* and dependencies show up for BlackDuck to generate a report on, and include in our distributed license file even though none of that is browser code that will be shipped to users.

🔬 Minimal Reproduction

  • Create a new project: ng create new-project
  • Add @angular/localize: ng add @angular/localize
@ngbot ngbot bot added this to the needsTriage milestone Aug 3, 2020
@JoostK
Copy link
Member

JoostK commented Aug 3, 2020

This is an interesting one. The proposed solution of listing @babel/core only as a dev dependency would not work, unfortunately. Doing it like that may result in @babel/core being unavailable, as dev dependencies are not installed in consuming projects.

This situation occurs because @angular/localize includes multiple targets: the localize runtime and the localize tooling. The latter needs the @babel/core dependency, whereas the former does not. Tooling dependencies typically go into a project's dev dependencies, such that the license extraction tool works correctly. In this case, however, it's typically a dependency of the project because of the runtime part.

I think you could workaround by changing @angular/localize into a dev dependency of your project. When building the project, the runtime parts of @angular/localize will be included in the build output so it should be okay not to list @angular/localize as a real dependency. The only potential issue with this could be with libraries when its consuming application does not depend on @angular/localize, as then the true dependency on @angular/localize will get lost.

@Airblader
Copy link
Contributor

@JoostK Thanks for clearing this up! Maybe long-term it makes sense to split @angular/localize into different packages? Given that the default way to install it is via schematic, both could be installed (in their appropriate category) anyway.

@JoostK
Copy link
Member

JoostK commented Aug 3, 2020 •
edited

@JoostK Thanks for clearing this up! Maybe long-term it makes sense to split @angular/localize into different packages? Given that the default way to install it is via schematic, both could be installed (in their appropriate category) anyway.

@Airblader Yeah, the way to go about it needs some discussion. Splitting could be one option, although I think it makes more sense to move the tooling part of @angular/localize into @angular/compiler-cli (which is already a peer dependency of @angular/localize). But there's possibly more alternatives, such as optionalDependencies (I'm not sure whether that could work, nor how e.g. license extraction tooling deal with optional deps).

@petebacondarwin
Copy link
Member

The original aim as to keep @angular/localize as separate from Angular as possible - so that it could feasibly be used standalone in a non-Angular project. But as it stands right now we needed to "borrow" some shared code from the compiler and compiler-cli so there is a dependency there. Also we want to keep the proliferation of new packages down, so we wanted to avoid having separate packages for the localize runtime and tooling.

But I agree that this is something we should consider going forward.

As it turns out, unless you are going to use "runtime" localization in your project then there is never a need for any of the @angular/localize package to be in your runtime. So @JoostK is correct that you could make it a devDependency in your project - even for libraries!

@ngbot ngbot bot modified the milestones: needsTriage, Backlog Aug 13, 2020
@petebacondarwin
Copy link
Member

So here is my plan:

  • change @angular/localize to install to devDependencies by default
  • update the ng add schematic to ask if the user intends to use $localize at runtime
  • if so, update the package.json to move the package to dependencies

I need to dig into schematics a bit to see if I can do this reasonably.

petebacondarwin added a commit to petebacondarwin/angular that referenced this issue Sep 2, 2020
@petebacondarwin
fix(localize): install @angular/localize in devDependencies by de…
171b5bb 
…fault

Previously this package was installed in the default `dependencies` section
of `package.json`, but this meant that its dependencies are treated as
dependencies of the main project - Babel, for example.

Generally, $localize` is not used at runtime - it is compiled out by the
translation tooling, so there is no need for it to be a full dependency.

This commit changes the default location of the package to be the
`devDependencies` section, but gives the user a prompt to choose
otherwise.

Fixes angular#38329
@petebacondarwin petebacondarwin mentioned this issue Sep 2, 2020
Closed
petebacondarwin added a commit to petebacondarwin/angular that referenced this issue Sep 2, 2020
@petebacondarwin
fix(localize): install @angular/localize in devDependencies by de…
7bae350 
…fault

Previously this package was installed in the default `dependencies` section
of `package.json`, but this meant that its own dependencies are treated as
dependencies of the main project: Babel, for example.

Generally, $localize` is not used at runtime - it is compiled out by the
translation tooling, so there is no need for it to be a full dependency.
In fact, even if it is used at runtime, the package itself is only used
at dev-time since the runtime bits will be bundled into a distributable.
So putting this package in `devDependencies` would only prevent libraries
from bringing the package into application projects that used them. This
is probably good in itself, since it should be up to the downstream project
to decide if it wants to include `@angular/localize` at runtime.

This commit changes the default location of the package to be the
`devDependencies` section, but gives an option `useAtRuntime` to choose
otherwise.

Fixes angular#38329
atscott pushed a commit that referenced this issue Sep 3, 2020
@petebacondarwin @atscott
fix(localize): install @angular/localize in devDependencies by de…
dbab744 
…fault (#38680)

Previously this package was installed in the default `dependencies` section
of `package.json`, but this meant that its own dependencies are treated as
dependencies of the main project: Babel, for example.

Generally, $localize` is not used at runtime - it is compiled out by the
translation tooling, so there is no need for it to be a full dependency.
In fact, even if it is used at runtime, the package itself is only used
at dev-time since the runtime bits will be bundled into a distributable.
So putting this package in `devDependencies` would only prevent libraries
from bringing the package into application projects that used them. This
is probably good in itself, since it should be up to the downstream project
to decide if it wants to include `@angular/localize` at runtime.

This commit changes the default location of the package to be the
`devDependencies` section, but gives an option `useAtRuntime` to choose
otherwise.

Fixes #38329

PR Close #38680
@atscott atscott closed this as completed in 50f4d8a Sep 3, 2020
profanis pushed a commit to profanis/angular that referenced this issue Sep 5, 2020
@petebacondarwin @profanis
fix(localize): install @angular/localize in devDependencies by de…
a1fce0f 
…fault (angular#38680)

Previously this package was installed in the default `dependencies` section
of `package.json`, but this meant that its own dependencies are treated as
dependencies of the main project: Babel, for example.

Generally, $localize` is not used at runtime - it is compiled out by the
translation tooling, so there is no need for it to be a full dependency.
In fact, even if it is used at runtime, the package itself is only used
at dev-time since the runtime bits will be bundled into a distributable.
So putting this package in `devDependencies` would only prevent libraries
from bringing the package into application projects that used them. This
is probably good in itself, since it should be up to the downstream project
to decide if it wants to include `@angular/localize` at runtime.

This commit changes the default location of the package to be the
`devDependencies` section, but gives an option `useAtRuntime` to choose
otherwise.

Fixes angular#38329

PR Close angular#38680
@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 Oct 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
area: i18n type: use-case
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

fix(localize): install @angular/localize in devDependencies by default petebacondarwin/angular
4 participants
@petebacondarwin @JoostK @sorahn @Airblader