build(bazel): use value of /// <amd-module name=“”> directive to convert fileNameToModuleName in ngc-wrapped #25650
Conversation
|
You can preview 22d4647 at https://pr25650-22d4647.ngbuilds.io/. |
gregmagolan
force-pushed
the
amd-module-name-for-ngfactory-v2
branch
from
22d4647
to
4b1e7a8
Compare
|
You can preview 4b1e7a8 at https://pr25650-4b1e7a8.ngbuilds.io/. |
gregmagolan
added
the
target: patch
|
Tested this, but I still have issues with our scoped package's secondary entrypoints since they consists of three name segments, i.e. "@connect/block/spinner". |
|
@gregmagolan I've tested with "@angular/cli": "~6.1.5" but having the same problem on every file change, Issue angular/angular-cli#11835
Edit: I'm using external library (demo-library) created with ng-packagr. I see the following error after first build (on every save):
|
|
@ease if you really want to test things right after they land, you need to use snapshot builds of Angular and CLI, e.g. use |
|
Still not quite right. Doesn’t handle secondary entry point module names such as In the meantime, angular/angular-cli#11835 is fixed at the expense of building angular from source with bazel downstream. |
…ert fileNameToModuleName in ngc-wrapped
gregmagolan
force-pushed
the
amd-module-name-for-ngfactory-v2
branch
from
4b1e7a8
to
eb69745
Compare
gregmagolan
changed the title
|
New approach after talking to @alexeagle and @alxhub. Using the moduleName from the ts.SourceFile will ensure the .ngfactory && .ngsummary files get the correct module names when building angular from source downstream |
|
See comment bazelbuild/rules_typescript#223 (comment) for a good trace of the issue. |
|
You can preview eb69745 at https://pr25650-eb69745.ngbuilds.io/. |
| @@ -226,6 +226,15 @@ export function compile({allowNonHermeticReads, allDepsCompiledWithBazel = true, | |||
| const ngHost = ng.createCompilerHost({options: compilerOpts, tsHost: bazelHost}); | |||
|
|
|||
| ngHost.fileNameToModuleName = (importedFilePath: string, containingFilePath: string) => { | |||
| try { | |||
There was a problem hiding this comment.
Should we check whether the file is a .d.ts file before attempting this?
Otherwise, LGTM.
There was a problem hiding this comment.
From slack:
@gregmagolan -> I thought of checking for .d.ts.. could a .ts have a /// <amd-module name="x" /> directive? and if it does do we care at that point?
@alexeagle -> it could, and I think the correct thing would be to honor that, since hopefully the user did it explicitly as a workaround for something
| if (sourceFile && sourceFile.moduleName) { | ||
| return sourceFile.moduleName; | ||
| } | ||
| } catch (err) { |
There was a problem hiding this comment.
Good point. We don't need humans to figure that out. Would you like to find/write a lint check that catches unused identifiers in catch block? Maybe we just need to turn one on.
…ert fileNameToModuleName in ngc-wrapped (angular#25650) PR Close angular#25650
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
The work-around that was removed in #25604 is still needed for building angular from bazel downstream. I was mistaken in my analysis.
This PR puts the workaround back but adds fixes the module name resolution for cases like
@angular/core.ngfactorywhich should actually be resolved to@angular/core/core.ngfactory.This will fix angular/angular-cli#11835 which promted #25604 while not breaking angular building from source downstream.
UPDATE: New approach after talking to @alexeagle and @alxhub. Using the moduleName from the ts.SourceFile will ensure the .ngfactory && .ngsummary files get the correct module names when building angular from source downstream