Skip to content

feat(ivy): implement UMD for ngcc#25445

Closed
petebacondarwin wants to merge 15 commits into
angular:masterfrom
petebacondarwin:ngcc-umd-host
Closed

feat(ivy): implement UMD for ngcc#25445
petebacondarwin wants to merge 15 commits into
angular:masterfrom
petebacondarwin:ngcc-umd-host

Conversation

@petebacondarwin
Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin commented Aug 12, 2018

Add support to ngcc for UMD formatted packages.

Sits on top of #29643

@petebacondarwin petebacondarwin added feature Label used to distinguish feature request from other issues action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Aug 12, 2018
@petebacondarwin petebacondarwin added this to the Ivy milestone Aug 12, 2018
@googlebot
Copy link
Copy Markdown

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that 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 here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

Comment thread packages/compiler-cli/src/ngcc/src/host/fesm2015_host.ts Outdated
Comment thread packages/compiler-cli/src/ngcc/test/helpers/utils.ts Outdated
Comment thread packages/compiler-cli/src/ngtsc/testing/in_memory_typescript.ts Outdated
@mary-poppins
Copy link
Copy Markdown

You can preview 215d9d2 at https://pr25445-215d9d2.ngbuilds.io/.

@mary-poppins
Copy link
Copy Markdown

You can preview a681c28 at https://pr25445-a681c28.ngbuilds.io/.

Copy link
Copy Markdown
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

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

Nice! 👏

Comment thread packages/compiler-cli/src/ngcc/src/host/umd_host.ts Outdated
Comment thread packages/compiler-cli/src/ngcc/src/host/umd_host.ts Outdated
Comment thread packages/compiler-cli/src/ngcc/src/host/umd_host.ts Outdated
Comment thread packages/compiler-cli/src/ngcc/src/host/umd_host.ts Outdated
Comment thread packages/compiler-cli/src/ngcc/src/host/umd_host.ts Outdated
Comment thread packages/compiler-cli/src/ngcc/test/host/esm5_host_spec.ts Outdated
@petebacondarwin petebacondarwin added state: WIP and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Aug 12, 2018
@petebacondarwin petebacondarwin changed the title feat(ivy): implement UmdReflectionHost for ngcc feat(ivy): implement UMD for ngcc Aug 12, 2018
@mary-poppins
Copy link
Copy Markdown

You can preview 03b3a45 at https://pr25445-03b3a45.ngbuilds.io/.

Comment thread packages/compiler-cli/src/ngcc/src/rendering/umd_renderer.ts Outdated
@petebacondarwin petebacondarwin force-pushed the ngcc-umd-host branch 2 times, most recently from 24356f0 to fb79996 Compare August 15, 2018 16:14
@mary-poppins
Copy link
Copy Markdown

You can preview fb79996 at https://pr25445-fb79996.ngbuilds.io/.

@mary-poppins
Copy link
Copy Markdown

You can preview c96ca1f at https://pr25445-c96ca1f.ngbuilds.io/.

@mary-poppins
Copy link
Copy Markdown

You can preview 96787b8 at https://pr25445-96787b8.ngbuilds.io/.

Copy link
Copy Markdown
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

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

👍

A couple more points about .has and using && and || as control flow.

It occurs to me that maybe I should write a TS style guide for the compiler, outlining the opinions I'm trying to enforce in the code, as well as the rationale for both of them.

Comment thread packages/compiler-cli/ngcc/src/host/umd_host.ts Outdated
Comment thread packages/compiler-cli/ngcc/src/rendering/dts_renderer.ts Outdated
Comment thread packages/compiler-cli/ngcc/src/rendering/dts_renderer.ts Outdated
Comment thread packages/compiler-cli/ngcc/src/host/esm2015_host.ts Outdated
Comment thread packages/compiler-cli/ngcc/src/rendering/dts_renderer.ts Outdated
@alxhub alxhub added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label May 13, 2019
@petebacondarwin
Copy link
Copy Markdown
Contributor Author

Agh! Sorry @alxhub I should have been more careful and fixed all of those.

@petebacondarwin petebacondarwin removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label May 14, 2019
@alxhub alxhub 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 May 14, 2019
@petebacondarwin
Copy link
Copy Markdown
Contributor Author

@jasonaden - conflicts resolved and rebased on master.

…rts`

Previously we were just checking that the object was "any" object but now
we check that it is the file object that we expected.
Previously we were using an anonymous type `{specifier: string; qualifier: string;}`
throughout the code base. This commit gives this type a name and ensures it
is only defined in one place.
The `getDeclaration()` function now searches down into the AST for
matching nodes, which is needed for UMD testing.
In UMD formats, imports are always namespaced. This commit makes
ngcc more tolerant of such structures.
Previously these fake files were full TypeScript source
files (`.ts`) but this is not necessary as we only need the
typings not the implementation.
In some cases the `forwardRef` helper has been imported via a namespace,
e.g. `core.forwardRef(...)`.

This commit adds support for unwrapping such namespaced imports when
ngtsc is statically evaluating code.
Previously the same `Renderer` was used to render typings (.d.ts)
files. But the new `UmdRenderer` is not able to render typings files
correctly.

This commit splits out the typings rendering from the src rendering.
To achieve this the previous renderers have been refactored from
sub-classes of the abstract `Renderer` class to  classes that implement
the `RenderingFormatter` interface, which are then passed to the
`Renderer` and `DtsRenderer` to modify its rendering behaviour.

Along the way a few utility interfaces and classes have been moved
around and renamed for clarity.
The dependency resolution that works out the order in which
to process entry-points must also understand UMD formats.
Previously we were relying upon the `.get()` method to return `undefined`
but it is clearer and safer to always check with `.has()` first.
@jasonaden jasonaden closed this in 8e201f7 May 16, 2019
jasonaden pushed a commit that referenced this pull request May 16, 2019
…25445)

Previously we were using an anonymous type `{specifier: string; qualifier: string;}`
throughout the code base. This commit gives this type a name and ensures it
is only defined in one place.

PR Close #25445
@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 cla: yes feature Label used to distinguish feature request from other issues 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.

5 participants