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

fix(ngcc): ignore format properties that exist but are undefined #32205

Closed

Conversation

@elvisbegovic
Copy link
Contributor

commented Aug 20, 2019

Should fix #32188

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

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:

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

would cause ngcc to throw:

ERROR in Invariant violated: No format-path or format for test-package ...

Issue Number: #32188

What is the new behavior?

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

Does this PR introduce a breaking change?

  • No

Other information

For reference, this regression was introduced in #32052.

/cc @gkalpak Is it possible to add some tests, please

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

@googlebot googlebot added the cla: yes label Aug 20, 2019

@gkalpak
Copy link
Member

left a comment

Thx 👍

Can you, please, update the commit message to adhere to our guidelines. It would, also, be useful to add some more context in the commit message; e.g. something like:

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
undefined) - 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

packages/compiler-cli/ngcc/src/main.ts Outdated Show resolved Hide resolved
packages/compiler-cli/ngcc/src/main.ts Show resolved Hide resolved

@ngbot ngbot bot modified the milestone: needsTriage Aug 20, 2019

@gkalpak gkalpak changed the title fix(compiler-cli): ignore first ivy process on properties that are not truthy evaluated fix(ngcc): ignore format properties that exist but are undefined 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 #32052.

Fixes #32188

@elvisbegovic elvisbegovic force-pushed the elvisbegovic:fix-ivy-lib-first-compilation branch from d3096c2 to 0bfccad Aug 20, 2019

@googlebot

This comment has been minimized.

Copy link

commented Aug 20, 2019

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Aug 20, 2019

@gkalpak gkalpak force-pushed the elvisbegovic:fix-ivy-lib-first-compilation branch from ddf2ca6 to 8d3be1d Aug 20, 2019

@gkalpak
Copy link
Member

left a comment

🎉

@gkalpak

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

@googlebot I consent.

@googlebot

This comment has been minimized.

Copy link

commented Aug 20, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Aug 20, 2019

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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.