Skip to content

fix(ivy): a collection of ngcc fixes for processing various 3rd party packages - #30591

Closed
petebacondarwin wants to merge 10 commits into
angular:masterfrom
petebacondarwin:ngcc-config
Closed

petebacondarwin wants to merge 10 commits into
angular:masterfrom
petebacondarwin:ngcc-config

Conversation

@petebacondarwin

@petebacondarwin petebacondarwin commented May 21, 2019 •

Copy link
Copy Markdown
Contributor

The good news is that with this PR I think we can process all the packages reported in #29564 (except for primeng).

Further, in order to process @angular/fire we need to provide the following file in your project root:

ngcc.config.js

module.exports = {
  packages: {
    '@angular/fire': {
      entryPoints: {
        './database-deprecated': {
          override: {
            'main': '../bundles/database-deprecated.umd.js',
          }
        },
      },
    },
  },
};

This is because the database-deprecated and database entry-points both point to the same source files. This is a bug in the package that should/will be fixed at the source.

Fixes #31069

@ngbot ngbot Bot added this to the needsTriage milestone May 21, 2019
@petebacondarwin
petebacondarwin force-pushed the ngcc-config branch 7 times, most recently from 0132dbe to cbe350d Compare May 22, 2019 19:28
@petebacondarwin
petebacondarwin marked this pull request as ready for review May 22, 2019 19:30
@petebacondarwin
petebacondarwin requested review from a team May 22, 2019 19:30
@ngbot ngbot Bot modified the milestones: needsTriage, Backlog May 22, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it a bit confusing that we use the same filename for both project and package configs. Why is that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it never occurred to me to call them something different... They are used in different places so it is their position that distinguishes them rather than their filename. Interested in @alxhub's view on this. I could be convinced to change it to ngcc.config.js for the project and ngcc.package.config.js??

@petebacondarwin petebacondarwin added the target: major This PR is targeted for the next major release label May 23, 2019
@ocombe ocombe added target: major This PR is targeted for the next major release and removed target: major This PR is targeted for the next major release labels May 23, 2019
@petebacondarwin
petebacondarwin force-pushed the ngcc-config branch 2 times, most recently from f0d92ad to 82f06bf Compare May 23, 2019 21:40
@petebacondarwin
petebacondarwin requested a review from a team May 23, 2019 21:40
@petebacondarwin
petebacondarwin requested a review from a team June 20, 2019 11:37
@petebacondarwin
petebacondarwin force-pushed the ngcc-config branch 9 times, most recently from 9211c65 to e484205 Compare June 24, 2019 12:01
@petebacondarwin
petebacondarwin removed the request for review from a team June 24, 2019 14:32

@alxhub alxhub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rebased this, updated paths, and changed the fix(ivy): configuration... commit to a feat commit.

@alxhub

alxhub commented Jun 25, 2019

Copy link
Copy Markdown
Member

Presubmit

Ivy Presubmit

@kara

kara commented Jun 26, 2019

Copy link
Copy Markdown
Contributor

@alxhub Thanks!

Now that ngcc uses a `FileSystem` throughout we no longer need
to rely upon mocking out the real file-system with mock-fs.
Previously each test relied on large shared mock file-systems, which
makes it difficult to reason about what is actually being tested.

This commit breaks up these big mock file-systems into smaller more
focused chunks.
There are scenarios where it is not possible for ngcc to guess the format
or configuration of an entry-point just from the files on disk.

Such scenarios include:

1) Unwanted entry-points: A spurious package.json makes ngcc think
there is an entry-point when there should not be one.

2) Deep-import entry-points: some packages allow deep-imports but do not
provide package.json files to indicate to ngcc that the imported path is
actually an entry-point to be processed.

3) Invalid/missing package.json properties: For example, an entry-point
that does not provide a valid property to a required format.

The configuration is provided by one or more `ngcc.config.js` files:

* If placed at the root of the project, this file can provide configuration
for named packages (and their entry-points) that have been npm installed
into the project.

* If published as part of a package, the file can provide configuration
for entry-points of the package.

The configured of a package at the project level will override any
configuration provided by the package itself.
Some packages do not actually provide a `typings` field in their
package.json. But TypeScript naturally infers the typings file from
the location of the JavaScript source file.

This commit modifies ngcc to do a similar inference when finding
entry-points to process.

Fixes angular#28603 (FW-1299)
Previously we expected the constructor parameter `decorators`
property to be an array wrapped in a function. Now we also support
an array not wrapped in a function.
…tBundle()`

This simplifies the interface somewhat but also allows us to make use of
other properties of the EntryPoint object in the future.
…dle` interface

This will allow users of the `EntryPointBundle` to use some of the `EntryPoint`
properties without us having to pass them around one by one.
It is part of `EntryPointBundle` so we can just use that, which
is generally already passed around.
Rather than passing a number of individual arguments, we can
just pass an `EntryPointBundle`, which already contains them.

This is also a precursor to using more of the properties in the bundle.
Our module resolution prefers `.js` files over `.d.ts` files because
occasionally libraries publish their typings in the same directory
structure as the compiled JS files, i.e. adjacent to each other.

The standard TS module resolution would pick up the typings
file and add that to the `ts.Program` and so they would be
ignored by our analyzers. But we need those JS files, if they
are part of the current package.

But this meant that we also bring in JS files from external
imports from outside the package, which is not desired.
This was happening for the `@fire/storage` enty-point
that was importing the `firebase/storage` path.

In this commit we solve this problem, for the case of imports
coming from a completely different package, by saying that any
file that is outside the package root directory must be an external
import and so we do not analyze those files.

This does not solve the potential problem of imports between
secondary entry-points within a package but so far that does
not appear to be a problem.
@angular-automatic-lock-bot

Copy link
Copy Markdown

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.

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: core Issues related to the framework runtime cla: yes effort2: days freq2: medium risk: medium target: major This PR is targeted for the next major release type: bug/fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

platform-server errors with ivy-ngcc

9 participants