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

refactor/fix(ivy): ngcc - a few refactorings and a fix #32052

Closed
wants to merge 7 commits into from

Conversation

@gkalpak
Copy link
Member

commented Aug 8, 2019

This PR mostly contains refactorings I did to get the code in a state that it is more easily parallelizable. Since these refactorings are good on their own (independently of parallelization), I thought it would be a good idea to split them out into a separate PR.

There is also a minor fix for correctly recording all new entry-points in package.json when createNewEntryPointFormats is true.

(See individual commit messages for details.)

gkalpak added 3 commits Aug 5, 2019
refactor(ivy): ngcc - fix return type on `makeEntryPointBundle()`
In commit 7b55ba5 (part of PR #29092), the implementation of
`makeEntryPointBundle()` was changed such that it now always return
`EntryPointBundle` (and not `null`).
However, the return type was not updated and as result we continued to
unnecessarily handle `null` as a potential return value in some places.

This commit fixes the return type to reflect the implementation and
removes the redundant code that was dealing with `null`.
refactor(ivy): ngcc - only try to process the necessary properties
This change basically moves some checks to happen up front and ensures
we don't try to process any more properties than we absolutely need.
(The properties would not be processed before either, but we would
consider them, before finding out that they have already been processed
or that they do not exist in the entry-point's `package.json`.)

This change should make no difference in the work done by `ngcc`, but it
transforms the code in a way that makes the actual work known earlier,
thus making it easier to parallelize the processing of each property in
the future.
@mary-poppins

This comment has been minimized.

Copy link

commented Aug 8, 2019

gkalpak added 3 commits Aug 7, 2019
refactor(ivy): ngcc - split work into distinct analyze/compile/execut…
…e phases

This refactoring more clearly separates the different phases of the work
performed by `ngcc`, setting the ground for being able to run each phase
independently in the future and improve performance via parallelization.

Inspired by/Based on @alxhub's prototype: alxhub/angular@cb631bd
refactor(ivy): ngcc - remove `formatProperty` from `EntryPointBundle`
Remove the `formatProperty` property from the `EntryPointBundle`
interface, because the property is not directly related to that type.

It was only used in one place, when calling `fileWriter.writeBundle()`,
but we can pass `formatProperty` directrly to `writeBundle()`.
refactor(ivy): ngcc - remove redundant `entryPoint` argument from `wr…
…iteBundle()`

The entry-point is already available through the `bundle` argument, so
passing it separately is redundant.

@gkalpak gkalpak force-pushed the gkalpak:more-ngcc-improvements branch from 22ab61d to 366c3ea Aug 8, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Aug 8, 2019

@gkalpak gkalpak marked this pull request as ready for review Aug 8, 2019

@gkalpak gkalpak requested a review from angular/fw-ngcc as a code owner Aug 8, 2019

fix(ivy): ngcc - correctly update `package.json` when `createNewEntry…
…PointFormats` is true

Previously, when run with `createNewEntryPointFormats: true`, `ngcc`
would only update `package.json` with the new entry-point for the first
format property that mapped to a format-path. Subsequent properties
mapping to the same format-path would be detected as processed and not
have their new entry-point format recorded in `package.json`.

This commit fixes this by ensuring `package.json` is updated for all
matching format properties, when writing an `EntryPointBundle`.

@gkalpak gkalpak force-pushed the gkalpak:more-ngcc-improvements branch from 366c3ea to 52ecf31 Aug 8, 2019

@mary-poppins

This comment has been minimized.

Copy link

commented Aug 8, 2019

@petebacondarwin
Copy link
Member

left a comment

Love it.. A few minor NITs that you can consider.

Also a typo in refactor(ivy): ngcc - remove formatProperty from EntryPointBundle: directrly -> directly

pull bot pushed a commit to Potapy4/angular that referenced this pull request Aug 8, 2019
pull bot pushed a commit to Potapy4/angular that referenced this pull request Aug 8, 2019
refactor(ivy): ngcc - only try to process the necessary properties (a…
…ngular#32052)

This change basically moves some checks to happen up front and ensures
we don't try to process any more properties than we absolutely need.
(The properties would not be processed before either, but we would
consider them, before finding out that they have already been processed
or that they do not exist in the entry-point's `package.json`.)

This change should make no difference in the work done by `ngcc`, but it
transforms the code in a way that makes the actual work known earlier,
thus making it easier to parallelize the processing of each property in
the future.

PR Close angular#32052
pull bot pushed a commit to Potapy4/angular that referenced this pull request Aug 8, 2019
refactor(ivy): ngcc - split work into distinct analyze/compile/execut…
…e phases (angular#32052)

This refactoring more clearly separates the different phases of the work
performed by `ngcc`, setting the ground for being able to run each phase
independently in the future and improve performance via parallelization.

Inspired by/Based on @alxhub's prototype: alxhub/angular@cb631bd

PR Close angular#32052
pull bot pushed a commit to Potapy4/angular that referenced this pull request Aug 8, 2019
refactor(ivy): ngcc - remove `formatProperty` from `EntryPointBundle` (
…angular#32052)

Remove the `formatProperty` property from the `EntryPointBundle`
interface, because the property is not directly related to that type.

It was only used in one place, when calling `fileWriter.writeBundle()`,
but we can pass `formatProperty` directrly to `writeBundle()`.

PR Close angular#32052
pull bot pushed a commit to Potapy4/angular that referenced this pull request Aug 8, 2019
refactor(ivy): ngcc - remove redundant `entryPoint` argument from `wr…
…iteBundle()` (angular#32052)

The entry-point is already available through the `bundle` argument, so
passing it separately is redundant.

PR Close angular#32052
gkalpak added a commit to gkalpak/angular that referenced this pull request Aug 19, 2019
refactor(ngcc): minor code clean-up following angular#32052
This commit addresses the review feedback from angular#32052, which was merged
before addressing the feedback there.
gkalpak added a commit to gkalpak/angular that referenced this pull request Aug 20, 2019
refactor(ngcc): minor code clean-up following angular#32052
This commit addresses the review feedback from angular#32052, which was merged
before addressing the feedback there.
elvisbegovic added a commit to elvisbegovic/angular that referenced this pull request Aug 20, 2019
fix(ngcc): ignore format properties that exist but are undefined
Previously, `ngcc` assumed that if a format property was defined in
`package.json` it would point to a valid format-path (i.e. a file that
is an entry-point for a specific format). This is generally the case,
except if a format property is set to a non-string value (such as
`package.json`) - either directly in the `package.json` (which is unusual)
or in ngcc.config.js (which is a valid usecase, when one wants a
format property to be ignored by `ngcc`).

For example, the following config file would cause `ngcc` to throw:

```
module.exports = {
  packages: {
    'test-package': {
      entryPoints: {
        '.': {
          override: {
            fesm2015: undefined,
          },
        },
      },
    },
  },
};
```

This commit fixes it by ensuring that only format properties whose value
is a string are considered by `ngcc`.

For reference, this regression was introduced in angular#32052.

Fixes angular#32188
gkalpak added a commit to gkalpak/angular that referenced this pull request Aug 20, 2019
refactor(ngcc): minor code clean-up following angular#32052
This commit addresses the review feedback from angular#32052, which was merged
before addressing the feedback there.
AndrewKushnir added a commit that referenced this pull request Aug 20, 2019
fix(ngcc): ignore format properties that exist but are undefined (#32205
)

Previously, `ngcc` assumed that if a format property was defined in
`package.json` it would point to a valid format-path (i.e. a file that
is an entry-point for a specific format). This is generally the case,
except if a format property is set to a non-string value (such as
`package.json`) - either directly in the `package.json` (which is unusual)
or in ngcc.config.js (which is a valid usecase, when one wants a
format property to be ignored by `ngcc`).

For example, the following config file would cause `ngcc` to throw:

```
module.exports = {
  packages: {
    'test-package': {
      entryPoints: {
        '.': {
          override: {
            fesm2015: undefined,
          },
        },
      },
    },
  },
};
```

This commit fixes it by ensuring that only format properties whose value
is a string are considered by `ngcc`.

For reference, this regression was introduced in #32052.

Fixes #32188

PR Close #32205
gkalpak added a commit to gkalpak/angular that referenced this pull request Aug 22, 2019
refactor(ngcc): minor code clean-up following angular#32052
This commit addresses the review feedback from angular#32052, which was merged
before addressing the feedback there.
gkalpak added a commit to gkalpak/angular that referenced this pull request Aug 26, 2019
refactor(ngcc): minor code clean-up following angular#32052
This commit addresses the review feedback from angular#32052, which was merged
before addressing the feedback there.
ngdevelop-tech added a commit to ngdevelop-tech/angular that referenced this pull request Aug 27, 2019
fix(ngcc): ignore format properties that exist but are undefined (ang…
…ular#32205)

Previously, `ngcc` assumed that if a format property was defined in
`package.json` it would point to a valid format-path (i.e. a file that
is an entry-point for a specific format). This is generally the case,
except if a format property is set to a non-string value (such as
`package.json`) - either directly in the `package.json` (which is unusual)
or in ngcc.config.js (which is a valid usecase, when one wants a
format property to be ignored by `ngcc`).

For example, the following config file would cause `ngcc` to throw:

```
module.exports = {
  packages: {
    'test-package': {
      entryPoints: {
        '.': {
          override: {
            fesm2015: undefined,
          },
        },
      },
    },
  },
};
```

This commit fixes it by ensuring that only format properties whose value
is a string are considered by `ngcc`.

For reference, this regression was introduced in angular#32052.

Fixes angular#32188

PR Close angular#32205
gkalpak added a commit to gkalpak/angular that referenced this pull request Aug 27, 2019
refactor(ngcc): minor code clean-up following angular#32052
This commit addresses the review feedback from angular#32052, which was merged
before addressing the feedback there.
gkalpak added a commit to gkalpak/angular that referenced this pull request Aug 27, 2019
refactor(ngcc): minor code clean-up following angular#32052
This commit addresses the review feedback from angular#32052, which was merged
before addressing the feedback there.
gkalpak added a commit to gkalpak/angular that referenced this pull request Aug 29, 2019
refactor(ngcc): minor code clean-up following angular#32052
This commit addresses the review feedback from angular#32052, which was merged
before addressing the feedback there.
gkalpak added a commit to gkalpak/angular that referenced this pull request Aug 29, 2019
refactor(ngcc): minor code clean-up following angular#32052
This commit addresses the review feedback from angular#32052, which was merged
before addressing the feedback there.
gkalpak added a commit to gkalpak/angular that referenced this pull request Aug 30, 2019
refactor(ngcc): minor code clean-up following angular#32052
This commit addresses the review feedback from angular#32052, which was merged
before addressing the feedback there.
@gkalpak gkalpak referenced this pull request Aug 30, 2019
3 of 3 tasks complete
gkalpak added a commit to gkalpak/angular that referenced this pull request Sep 5, 2019
refactor(ngcc): minor code clean-up following angular#32052
This commit addresses the review feedback from angular#32052, which was merged
before addressing the feedback there.
gkalpak added a commit to gkalpak/angular that referenced this pull request Sep 5, 2019
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
refactor(ivy): ngcc - fix return type on `makeEntryPointBundle()` (an…
…gular#32052)

In commit 7b55ba5 (part of PR angular#29092), the implementation of
`makeEntryPointBundle()` was changed such that it now always return
`EntryPointBundle` (and not `null`).
However, the return type was not updated and as result we continued to
unnecessarily handle `null` as a potential return value in some places.

This commit fixes the return type to reflect the implementation and
removes the redundant code that was dealing with `null`.

PR Close angular#32052
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
refactor(ivy): ngcc - only try to process the necessary properties (a…
…ngular#32052)

This change basically moves some checks to happen up front and ensures
we don't try to process any more properties than we absolutely need.
(The properties would not be processed before either, but we would
consider them, before finding out that they have already been processed
or that they do not exist in the entry-point's `package.json`.)

This change should make no difference in the work done by `ngcc`, but it
transforms the code in a way that makes the actual work known earlier,
thus making it easier to parallelize the processing of each property in
the future.

PR Close angular#32052
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
refactor(ivy): ngcc - split work into distinct analyze/compile/execut…
…e phases (angular#32052)

This refactoring more clearly separates the different phases of the work
performed by `ngcc`, setting the ground for being able to run each phase
independently in the future and improve performance via parallelization.

Inspired by/Based on @alxhub's prototype: alxhub/angular@cb631bd

PR Close angular#32052
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
refactor(ivy): ngcc - remove `formatProperty` from `EntryPointBundle` (
…angular#32052)

Remove the `formatProperty` property from the `EntryPointBundle`
interface, because the property is not directly related to that type.

It was only used in one place, when calling `fileWriter.writeBundle()`,
but we can pass `formatProperty` directrly to `writeBundle()`.

PR Close angular#32052
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
refactor(ivy): ngcc - remove redundant `entryPoint` argument from `wr…
…iteBundle()` (angular#32052)

The entry-point is already available through the `bundle` argument, so
passing it separately is redundant.

PR Close angular#32052
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
fix(ivy): ngcc - correctly update `package.json` when `createNewEntry…
…PointFormats` is true (angular#32052)

Previously, when run with `createNewEntryPointFormats: true`, `ngcc`
would only update `package.json` with the new entry-point for the first
format property that mapped to a format-path. Subsequent properties
mapping to the same format-path would be detected as processed and not
have their new entry-point format recorded in `package.json`.

This commit fixes this by ensuring `package.json` is updated for all
matching format properties, when writing an `EntryPointBundle`.

PR Close angular#32052
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
fix(ngcc): ignore format properties that exist but are undefined (ang…
…ular#32205)

Previously, `ngcc` assumed that if a format property was defined in
`package.json` it would point to a valid format-path (i.e. a file that
is an entry-point for a specific format). This is generally the case,
except if a format property is set to a non-string value (such as
`package.json`) - either directly in the `package.json` (which is unusual)
or in ngcc.config.js (which is a valid usecase, when one wants a
format property to be ignored by `ngcc`).

For example, the following config file would cause `ngcc` to throw:

```
module.exports = {
  packages: {
    'test-package': {
      entryPoints: {
        '.': {
          override: {
            fesm2015: undefined,
          },
        },
      },
    },
  },
};
```

This commit fixes it by ensuring that only format properties whose value
is a string are considered by `ngcc`.

For reference, this regression was introduced in angular#32052.

Fixes angular#32188

PR Close angular#32205
gkalpak added a commit to gkalpak/angular that referenced this pull request Sep 7, 2019
refactor(ngcc): minor code clean-up following angular#32052
This commit addresses the review feedback from angular#32052, which was merged
before addressing the feedback there.
gkalpak added a commit to gkalpak/angular that referenced this pull request Sep 7, 2019
matsko added a commit that referenced this pull request Sep 9, 2019
refactor(ngcc): minor code clean-up following #32052 (#32427)
This commit addresses the review feedback from #32052, which was merged
before addressing the feedback there.

PR Close #32427
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

commented Sep 15, 2019

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 Sep 15, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.