Skip to content

Conversation

@devversion
Copy link
Member

@devversion devversion commented Oct 5, 2021

Switches from View Engine NPM package output to the partial
compilation-based NPM outout. As part of this, various
workarounds/patches to make the View Engine release output
work with Bazel have been removed (finally!)

See individual commits.. (commits could potentially be more fine-grained
and more well-documented but I'm running out of time at this point)

@devversion devversion added the in progress This issue is currently in progress label Oct 5, 2021
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 5, 2021
@devversion devversion force-pushed the apf-v13 branch 8 times, most recently from b09fca0 to e3bf269 Compare October 6, 2021 18:50
@devversion devversion added the target: major This PR is targeted for the next major release label Oct 6, 2021
@devversion devversion added this to the 13.0.0 milestone Oct 6, 2021
@devversion devversion added the P2 The issue is important to a large percentage of users, with a workaround label Oct 6, 2021
@devversion devversion force-pushed the apf-v13 branch 2 times, most recently from ed0ad06 to 9b0f6c1 Compare October 6, 2021 20:25
@devversion devversion added merge: preserve commits When the PR is merged, a rebase and merge should be performed and removed in progress This issue is currently in progress labels Oct 6, 2021
@devversion devversion marked this pull request as ready for review October 6, 2021 20:26
Copy link
Member

@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

Just two nits around a command and type.

@devversion devversion force-pushed the apf-v13 branch 2 times, most recently from 8043f55 to cf28bf3 Compare October 6, 2021 22:31
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM, overall looks great (especially all of the gulp cleanup), just a handful of nits, you can add "merge ready" when ready

####################################
# Bazel custom flags #
####################################
build --flag_alias=partial_compilation=@npm//@angular/bazel/src:partial_compilation
Copy link
Member

Choose a reason for hiding this comment

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

Is this something we'll remove before long? If so, should we add a TODO to remove? Similar for the ivy flag below.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should exist forever (as long as there is no new concept of library output).

Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
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.

I went over most of this pretty quickly, but this looks fantastic to me 👏 . Thanks for putting in the effort of separating this into digestible commits. Exciting to see all VE workarounds, the CI jobs and ngcc integration jobs all disappear along with the introduction of ESBuild!

Switches the devmode output to ES2020 and ESM. This is in preparation
for allowing fast rebuilds with the Angular FW v13 packages.

Since these packages are now strict ESM, and rely on the linker, we need
to have some bundling in between for tests, or the dev-app (this is
actually even faster than relying on ConcatJS/RequireJS/SystemJS).

This bundling should not need to consume the prodmode output, but rather
use the devmode output because otherwise the usual development workflow
would result in both devmode + prodmode being requested.. resulting in a
significant slow-down due to 2x TS compilations. Devmode output is
always requested for dependencies on `ts_library` or `ng_module` to
accomplish the TS fine-grained TS compilations. Since this is the case,
we just naturally use the devmode output for bundling of tests/dev-app..
so inherently we need to switch devmode to ESM for enabling bundling.
For APF v13, we have removed the `entry_point` attribute from the
`ng_package` rule. This attribute did not do what it described. i.e. it
was not responsible for determining the primary package entry-point.

This commit removes all of these usages as they are no longer valid,
nor needed.
For APF v13, we will switch to the custom devserver we have used for
the dev-app as well. We do this because the ConcatJS devserver is no
longer needed due to us bundling the e2e-app.

Note again: This is needed because FW packages do no longer ship UMD
files, and we also need to consolidate our pre-built linker ESM bundles
of the Angular framework packages. A bundler like ESBuild will do that
job for us. It's fast enough that it's subjectively significantly faster
than ConcatJS/RequireJS resolving deps at runtime in the browser..
…ser tests

Replaces the legacy gulp setup with a simple script that runs the
Angular compiler and ESBuild with the linker. This is simpler than
switching the Gulp legacy output to properly deal with the FW packages
using partial compilation output together with SystemJS.

It has been a long goal removing SystemJS and the gulp tooling anyway,
so this is a good chance for simplifying this code.. removing the need
for all these additional tsconfig files; and additional configuration of
the legacy gulp tooling when wiring up new packages.
Updates the linker integration test to no logner use deep imports
into the stict ESM package of `@angular/compiler-cli`. The linker
integration test tooling is now using ESM as well; in order to
be able to import into the compiler-cli.
Updates the build setup to no longer support the View Engine
mode through a Bazel define flag.
Previously, the dev-app relied on SystemJS for resolving all files. This
required a lot of configuration and was very cumbersome to maintain.

This commit switches both serve-targets to use ESBuild instead. ESbuild
will resolve all the dependencies for us at build time; removing the
manual configuration work, and it will also allow us to incorporate the
linked framework ESM bundles, so that the apps would not need the
Angular compiler at runtime (for JIT).

Generally it seems that ESBuild is extremely fast so that it makes
up for the runtime resolution through SystemJS; and manual configuration
work. It also makes our tooling more modern and easier to adopt to new
changes (e.g. if we'd add a dependency like `date-fns` there is no
special tooling needed).
Similar to the reason why we switched the dev-app and e2e-app to use
ESBuild, we will use ESBuild as a step in between for running browser
Karma tests, or running e2e protractor/selenium-webdriver tests.

This allows us to incorporate the framework linked ESM bundles and
helps working around the change that no UMD bundles are available for
APF v13 packages. Running ESBuild as a build step in between seems
non-significant as on the other hand the module resolution in the
browser for Karma seemed noticeable slower (due to it loading hundreds
of files individually); and also the Karma browser setup required more
manual maintenance (through RequireJS configurations).
This commit updates the pre-render test to work with APF v13 package
output which is strict ESM. Since the test runs inside Node, we would
need to update our test to run in ESM as well. This is not possible
with the Bazel NodeJS rules yet, and also there are a couple of more
issues with running ESM devmode directly in NodeJS:

* ESM requires an explicit extension for releative imports. TypeScript
  will not add an explict extension so this breaks for our local package
  output. A bundler is needed and this is what the CLI will do for SSR
  as well.

* We want the Angular linked ESM bundles for framework packages as these
  are only partially compiled. This is not possible to do without
  modifiyng the node modules / or using a bundler that in-memory
  modifies the resolved framework packages (like the CLI does with its
  webpack plugins).
There were a couple of code places which started breaking with us
switching the devmode/prodmode target and mode to ES2020:

* Since devmode is now ES2020, arrow functions are no longer downleveled
  for tests to actual `function` declarations. This breaks spies in
  google-maps or the youtube-player package as the tests try to
  instantiate a spy using `new`. An arrow function cannot be
  instantiated through `new`.. so we manually update these spies to not
  use arrow functions.

* The MDC-based dialog harness extended from the non-MDC dialog harness
  and the `with` static member conflicted. It's unclear why this in
  particular happened but it was straightforward enough to just
  introduce a base class for the dialog harness (like it is done in
  other places as well).
Removes all the ngcc compatibility CI jobs as the APF v13 output
no longer comes with View Engine output and makes ngcc a noop for
the Angular Component package output.
Removes the unused `linker-process` Bazel rule. We no longer need this
AMD-based Bazel transformation/middleman rule as we can directly run the
linker when bundling unit tests.
The CLI does not yet support running schematics as ESM. For this
we actually still emit CommonJS output for schematics. We need to
add a sub-directory `package.json` for the schematic folders though
as otherwise NodeJS would incorrectly treat the `.js` schematic files
as ESM due to the top-level package.json file setting `type: module`.
Adds a subpath export for the `fesm2020` folder of the internal
`@angular/components-examples` package. This subpath is needed so that
Webpack can generate lazy-loadable chunks for all examples in the
documentation app.
@devversion devversion added action: merge The PR is ready for merge by the caretaker P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful and removed P2 The issue is important to a large percentage of users, with a workaround labels Oct 7, 2021
Adds the `exports` field for module resolution of `@angular/cdk`
and `@angular/material` when Sass is requested.

Since these packages use the `exports` field already, to prevent
deep imports, we also need to expose the Sass entry-points and prebuilt
files. The webpack loader requires the `sass` conditional to be set
when the CDK or Material modules are loaded without the `includePaths` option.

It is worth noting that the Sass webpack loader does not seem to respect
the exports field when `@import` is used. Still for consistency and
other potential bundlers/loaders, we will expose the `.css` extension
subpaths. Also as a side-note: the `.css` extension imports do not
work with `@use`, so the non-extension variants are helpful there for
backwards compatibility & convenient access.
Updates the Node version used in Bazel so that it does
not fail with the minimium node version from Angular FW packages.
@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 Nov 7, 2021
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 PR author has agreed to Google's Contributor License Agreement merge: preserve commits When the PR is merged, a rebase and merge should be performed P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants