fix(bazel): ng_module rule does not expose flat module information in Ivy

[devversion]

The ng_module rule supports the generation of flat module bundles. In
View Engine, information about this flat module bundle is exposed
as a Bazel provider. This is helpful as other rules like ng_package
could rely on this information to determine entry-points for the APF.

With Ivy this currently does not work because the flat module
information is not exposed in the provider. The reason for this is
unclear. We should also provide this information in Ivy so that rules
like ng_package can also determine the correct entry-points when a
package is built specifically with --config=ivy.

This allows us re-enable the tests we previously had to disable when switching to APF. The tests started failing because previously the entry-points were hardcoded, but now we rely on the automatic resolution of those. And apparently tests highlighted that the resolution is incorrect in Ivy.

[devversion]

This is blocked on angular/components#19276 so that the components repo job can turn green. Review on this PR would be still appreciated though 😄

[JoostK]

I'm delegating the compiler approval as I don't know about any of this stuff. Btw, this is currently failing CI even without the components tests.

[IgorMinar]

We should discuss this one in person with @alxhub as there as several things at play here.

I looked into this in the past and it wasn't quite clear what we should about the issue you are highlighting.

This was primarily because it was not even clear if we needed this feature at all or if we could simply if significantly.

In the past when the VE compiler was compiling a library into npm-shippable format, it also had to re-export private symbols referenced by component metadata within this lib via the flat index file. These reexports were necessary so that these symbols could be imported by factories produced much later by the second compiler pass invoked from within the app that used this library.

With ivy we no longer need these private symbol reexports because the factories are inlined into the original classes. In fact the final ivy npm package (g.co/apf) format will be different than what it is today but these plans have not solidified yet and Alex is doing exploration in this area.

One possible resolution for this PR could be that we merge this change as a temporary improvement that will likely be replaced/augmented further by Alex's changes before we actually start shipping this format to npm.

[devversion]

@IgorMinar I agree that long-term this needs to be reconsidered (i.e. packaging w/ Ivy in general), but in the current state, ng_module generates a flat module index (even with --config=ivy), so one would expect that generated flat module file to be used in the ng_package output.

Side note: We already use ng_package with --config=ivy for our components docs examples.

Not using the flat module index has the downside that index paths need to be guessed in the ng_package rule while actually there is information for the entry-point file available when the flat module index is built. Also generally, if we look at this only from the ng_module perspective, then it makes sense to expose that information similar to how it is done with View Engine. We should try to not conditionally have varying providers for ng_module targets as that complicates provider access for custom rules/tooling in Bazel (like ng_package).

In a current Bazel project, switching between compilation modes should be possible. So, even if flat module indices are not technically needed with the Ivy configuration, they are needed as a layer of indirection to make the switch between --config possible (and this also involves that providers do not change unless really needed. Otherwise tooling could break in one configuration or the other. Like in our ng_package tests)

Happy to discuss this more. Let me know if this should happen in the PR, or if you want set up a meeting.

[josephperrott]

If I am understanding this correctly, it seems that having ng_module export flag module info could be considered part of ensuring backward compatibility with Ivy.

[devversion]

@josephperrott Yeah, that seems correct to me.

[alxhub]

This change on the surface looks okay to me. It does what it says on the box :)

There's a larger issue here, though, that I think we should discuss before going down this route.

Eventually we want to switch away from ng_module.bzl entirely, and compile Angular packages as ts_library invocations with an Angular plugin. Ideally in most cases this will be a switch we flip inside the ng_module alias, but it seems like the ng_package machinery relies on special knowledge of Angular outputs in order to function. Given that this information won't be available from ts_library outputs, I think the right fix to this problem is to make ng_package work in the absence of any special information about its inputs. I recognize that's not an easy problem to solve, but I think we should look at what that would take.

[devversion]

@alxhub That's a really good point. I agree that ng_package long-term needs to be able to handle this without information provided by ng_module.

Also agreed that this is probably more difficult than it initially sounds. How do you want to proceed on this? My thinking is that this change is still not necessarily coupled to ng_package but rather aligns providers for ng_module (which would good to fix anyway for interim) and there would be no concern with landing this in the meanwhile.

I'd be happy to help with this issue in the long-term.

[josephperrott]

I'm good with the changes from a dev-infra perspective, but will let @IgorMinar sign off for bazel as well.

[IgorMinar]

This looks good to me, except that I'd like is to move away from the "flat module" terminology in this code. The provider doesn't provide info about flat modules, but rather about "entryPoints".

Historically we often confused the "flat module" and "entry point" terminology because it was complicated by odd requirements that View Engine had.

For the historical context: the index file used to create/bundle the flat module file, or FESM, had to be generated because of reexports of private symbols from within the flattened module to ensure that factories built by ngc didn't have deep imports into the flattened module. These deep imports would not work with FESMs and the generated reexports ensured that everything still worked. It was a terrible hack, but thankfully as soon as VE is deleted we'll no longer have to think about it.

In the meantime, I strongly suggest that we rename the terminology we use in these providers to entry points. I'm fine ok if this is done in a follow up PR as long as it's done eventually. Thanks

[devversion]

@IgorMinar Yeah, that's a good point. One concern I'd have with changing this to entryPoints is that we technically cannot, and do not infer the entry-point. The only information ng_module has is that a flat module file is being generated. In most cases this is the actual entry-point, that is right and ng_package assumes that.

Though long-term, if we get to the point with Ivy where flat module files are no longer generated, we won't have a good way to determine an entry-point file (unless we add some configuration-level heuristics or an other attribute like entry_point). So it might not be helpful having metadata exposed for entryPoints then. The current provider construct is accurately concerned with flat module files that could be reasonably removed with the removal of VE I assume.

I feel like it would be best to just keep terminology as is (namely flat module metadata) and revisit once VE is gone. I assume once we get there, it will involve more thinking on how ng_package (and not ng_module) could determine entry-points. This has been mentioned above by Alex too. Ideally (with flat module files gone), we'd shift the responsibility of entry point determination to ng_package, so that ts_library targets also work properly. They cannot not expose any entry-point information either.

[IgorMinar]

@devversion sgtm

I do think that the flatModuleId and flatModuleOutFile compiler options should be deprecated as at the time we release Ivy support for libraries because those options don't make sense with Ivy and should be supported by Ivy just to easy the migration from VE to Ivy for libraries.

https://angular.io/guide/angular-compiler-options#flatmoduleid

[alxhub]

Presubmit

[angular-automatic-lock-bot]

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.}