Skip to content

Commit

Permalink
fix(@schematics/angular): improve paths and exclude options
Browse files Browse the repository at this point in the history 
Currently the library schematic doesn't support adding a secondary entry-point and having deep imports is not recommanded.

It's best if paths are more stricter when having a secondary entry-point instead of a wildcard.

Instead of :
```
"lib/*": [
  "dist/lib/*"
]
```

Users should configure:
```
"lib/secondary": [
  "dist/lib/secondary"
]
```

This would allow a better DX experience when using auto imports in IDE's.

Closes: #15952
  • Loading branch information
@alan-agius4 @mgechev
alan-agius4 authored and mgechev committed Dec 3, 2019
1 parent cc0d5fc commit be1bcba
Show file tree
Hide file tree
Showing 3 changed files with 1 addition and 11 deletions.
7 changes: 0 additions & 7 deletions packages/schematics/angular/library/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,6 @@ function updateTsConfig(packageName: string, distRoot: string) {
tsconfig.compilerOptions.paths[packageName] = [];
}
tsconfig.compilerOptions.paths[packageName].push(distRoot);

// deep import & secondary entrypoint support
const deepPackagePath = packageName + '/*';
if (!tsconfig.compilerOptions.paths[deepPackagePath]) {
tsconfig.compilerOptions.paths[deepPackagePath] = [];
}
tsconfig.compilerOptions.paths[deepPackagePath].push(distRoot + '/*');
});
};
}
Expand Down
3 changes: 0 additions & 3 deletions packages/schematics/angular/library/index_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,9 +208,6 @@ describe('Library Schematic', () => {
expect(tsConfigJson.compilerOptions.paths.foo).toBeTruthy();
expect(tsConfigJson.compilerOptions.paths.foo.length).toEqual(1);
expect(tsConfigJson.compilerOptions.paths.foo[0]).toEqual('dist/foo');
expect(tsConfigJson.compilerOptions.paths['foo/*']).toBeTruthy();
expect(tsConfigJson.compilerOptions.paths['foo/*'].length).toEqual(1);
expect(tsConfigJson.compilerOptions.paths['foo/*'][0]).toEqual('dist/foo/*');
});

it(`should append to existing paths mappings`, async () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/schematics/angular/workspace/files/tsconfig.json.template
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
"compileOnSave": false,
"compilerOptions": {
"baseUrl": "./",
"outDir": "./dist/out-tsc",<% if (strict) { %>

This comment has been minimized.

Sign in to view

Copy link
@dmorosinotto

dmorosinotto Jan 19, 2020 •
edited
Loading

Contributor

Hi @alan-agius4 can I ask you why did you remove the /out-tsc from the "outDir" param in the root tsconfig.json?
I think that it's not related to your changes in updateTsConfig because this is NOT used to pass the value to distRoot parameter as you can see from the source const distRoot = `dist/${folderName}`; L194 that is passed in the call options.skipTsConfig ? noop() : updateTsConfig(packageName, distRoot), L217 so I don't understand why it's needed?

This cause a regression in VSCode when you create a new workspace with a library project and you run a script that do a build --watch of that library. VSCode loose the reference of imports pointing to library dist!

Can you please give some clarification?

I'll later open an issue to repro the failing behaviour in VSCode and if needed I'll try to fix it with a PR that revert the old path in tsconfig.json.

This comment has been minimized.

Sign in to view

Copy link
@alan-agius4

alan-agius4 Jan 19, 2020

Author Collaborator

The out-tsc was removed so that the entire dist folder gets excluded by tsc.

More context: https://www.typescriptlang.org/docs/handbook/tsconfig-json.html

This comment has been minimized.

Sign in to view

Copy link
@alan-agius4

alan-agius4 Jan 19, 2020 •
edited
Loading

Author Collaborator

VSCode loose the reference of imports pointing to library dist!

Which syntax are you using to import your library?

This comment has been minimized.

Sign in to view

Copy link
@dmorosinotto

dmorosinotto Jan 19, 2020 •
edited
Loading

Contributor

The out-tsc was removed so that the entire dist folder gets excluded by tsc.

@alan-agius4 This is the real problem!
If you point outDir to "/dist" you esclude everything that is produced in it,
and by default when you add library in an angular workspace (running for ex: ng generate library my-lib) it configure the "paths" in root tsconfig.json to resolve the library to the output of the build in /dist/my-lib

// inside root tsconfig.json
"paths": {
      "my-lib": ["dist/my-lib"]
}

and so with your change this cause an annoying behaviour when you are developing the app + lib (using a terminal that run ng build --project=my-lib --watch) that cause VSCode to loose all the reference each time the lib is rebuilt!

See this GIF (made by @damianog) for a repro, hope this can help understand the problem ;-)
outDir

This comment has been minimized.

Sign in to view

Copy link
@dmorosinotto

dmorosinotto Jan 19, 2020 •
edited
Loading

Contributor

PS: this problem is related to the new CLI v9.0.0-rc*
I've tried the same steps with CLI ~8.3.23 and it works because with the older version the tsconfig.json point to "outDir": "./dist/out-tsc"

This comment has been minimized.

Sign in to view

Copy link
@alan-agius4

alan-agius4 Jan 19, 2020

Author Collaborator

Thanks for investigating this. Mind also opening an issue? I’ll try to take another look to see why this is happening and if it’s a regression by another party such as TypeScript or VsCode.

As including the dist folder will also cause regressions such as the IDE suggesting incorrect imports. Also there might be the possibility to do a fix in ng-packagr so that it doesn’t delete the files on every partial recompilation.

This comment has been minimized.

Sign in to view

Copy link
@dmorosinotto

dmorosinotto Jan 19, 2020 •
edited
Loading

Contributor

Thanks for investigating this. Mind also opening an issue?

Yes I think to open an issue and even a related PR, if you think that this is an acceptable fix to the regression

I’ll try to take another look to see why this is happening and if it’s a regression by another party such as TypeScript or VsCode.

I think that it's a TypeScript and VSCode expected behaviour because as you pointed to me the tsconfig docs says that it explicit esclude (I think even for watching) the outDir path and this cause the annoying behaviour of not catching the new files after delete/rebuild process done by the ng build of the library in watch mode, even if we had an explicit paths including it...

As including the dist folder will also cause regressions such as the IDE suggesting incorrect imports. Also there might be the possibility to do a fix in ng-packagr so that it doesn’t delete the files on every partial recompilation.

I don't understand the indeep effects of ng-packagr, and I know that the IDE auto-import suggestion gives double paths for all the module/service/components exported from the lib ... but having all the imports code brakes everywhere at each build it is worst IMHO (especially because those are NOT errors!)

Hope you can find a better fix for the auto-import IDE integration, but right now I'll open issue and PR and then waiting for the merge or possible future improvement (before v9 release) 😉

This comment has been minimized.

Sign in to view

Copy link
@alan-agius4

alan-agius4 via email Jan 19, 2020 •
edited
Loading

Author Collaborator

This comment has been minimized.

Sign in to view

Copy link
@alan-agius4

alan-agius4 Jan 19, 2020

Author Collaborator

Bdw good investigation 😄.

This comment has been minimized.

Sign in to view

Copy link
@dmorosinotto

dmorosinotto Jan 19, 2020 •
edited
Loading

Contributor

Unfortunately the fix for auto imports, or imports suggestions is exclude the dist.

😢 😢 😢

I am not sure it this is truly expected, since with the same argument if you do a yarn install or yarn add you will experience the same behaviour since node_modules directory is excluded by default and on every yarn install/add the entire node_modules directory will be deleted.

🤔 well erasing or re-installing all the node_modules maybe is a "relative" infrequent situation, re-building the library of a working project maybe is more frequent 😉

Bdw good investigation 😄.

Thanks @alan-agius4 !
It was not simple to find the real cause of the issue, but my buddy @damianog help me with it, expecially for the video repro that he made!
And because he insisted that was something related to my investigation on CLI v9 because in his projects using CLI v8 there was NO problem at all, so we do an in-deep diff of our solution and found the blamed code 😉

This comment has been minimized.

Sign in to view

Copy link
@damianog

damianog Jan 19, 2020

@alan-agius4 We have opened the issue #16708

This comment has been minimized.

Sign in to view

Copy link
@dmorosinotto

dmorosinotto Jan 19, 2020

Contributor

Thanks @damianog for your help!
And this is the related PR: #16709
"outDir": "./dist",<% if (strict) { %>
"noImplicitAny": true,
"noImplicitReturns": true,
"noImplicitThis": true,
Expand Down

0 comments on commit be1bcba

Please sign in to comment.