Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(@schematics/angular): regression tsconfig.json #16708 #16709

Merged
merged 4 commits into from
Jan 24, 2020

Conversation

dmorosinotto
Copy link
Contributor

fix(@schematics/angular): regression tsconfig.json …
Unverified
0e318ae
Regression in tsconfig.json set "outDir": "./dist/out-tsc" for problems in VSCode TS(2307) when building library referred in tsconfig "paths"

Closes: #16708

fix(@schematics/angular): regression tsconfig.json  …
Unverified
0e318ae
Regression in tsconfig.json set `"outDir": "./dist/out-tsc"` for problems in VSCode TS(2307) when building library referred in tsconfig "paths" 
 
Closes: angular#16708
dmorosinotto referenced this pull request Jan 19, 2020
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
@alan-agius4
Copy link
Collaborator

Thanks for this.

@clydin and myself looked at this and it seems that TS file watcher is not handling deletion well when excludes are involved.

While checking this I also saw some change in behaviour which involved path mappings which seems that auto imports were no longer being suggested.

Would you also mind to fix the following

tsconfig.compilerOptions.paths[packageName].push(distRoot);
so that itgenerates the below paths;

    "paths": {
      "foo": [
        "dist/foo/foo",
        "dist/foo"
      ]
    }

instead of:

    "paths": {
      "foo": [
        "dist/foo"
      ]
    }

The first path will be used by TSC, while the second will be used by Webpack.

@dmorosinotto
Copy link
Contributor Author

Would you also mind to fix the following

@alan-agius4 Ok I'll update the code to add the second ref in the paths.
Do you think that it'll be acceptable to pass another param (the folderName) to the updateTSConfig function to do this?

@alan-agius4
Copy link
Collaborator

@dmorosinotto, no need as the packageName is the folder name.

@dmorosinotto
Copy link
Contributor Author

dmorosinotto commented Jan 20, 2020

@dmorosinotto, no need as the packageName is the folder name.

Well not exactly because if you pass a scoped name (like @myScope/myLib) due to strings.dasharize + scopeName handling if you use the packageName to write something like
tsconfig.compilerOptions.paths[packageName].push(distRoot + '/' + packageName);

You'll have this output :

"paths": {
      "@my-scope/my-lib": [
        "dist/my-scope/my-lib",
        "dist/my-scope/my-lib/@my-scope/my-lib"
      ]
 } 

You can try to test it in this blitz

that I suspect to be not exactly what you want, I think that this may be the correct one:

"paths": {
      "@my-scope/my-lib": [
        "dist/my-scope/my-lib",
        "dist/my-scope/my-lib/my-scope-my-lib"  //<-- note here NO @ and replace '/' with '-'
      ]
} 

JFYI I tryed to create a library with ng generate library @myScope/myLib and then compile it with ng build @myScoped/myLib with the current latest CLI 9.0.0.rc-9 and this is the output in /dist I don't know exactly which path Webpack will need?
AFTER ng build @myScope:myLib

@alan-agius4 Can you clarify me the real expected behavior? so I'll code the need changes, thanks!

@alan-agius4
Copy link
Collaborator

alan-agius4 commented Jan 20, 2020

You are right the paths should be without @ and - and any other special symbol.

Also the order of the below is important, as changing the order will result in a different behaviour.

paths": {
      "@my-scope/my-lib": [
        "dist/my-scope/my-lib/my-scope-my-lib", -> used by the IDE to suggest correct imports.
        "dist/my-scope/my-lib" -> used by the apps Webpack as otherwise the compilation will not succede because the above points to a dts file
      ]
} 

Improve paths in root tsconfig.json for better DX experience when using auto imports in IDE's.

Closes angular#16709
Improve paths in root tsconfig.json for better DX experience when using auto imports in IDE's.
Fix code lint.

Closes angular#16709
@dmorosinotto
Copy link
Contributor Author

Sorry @alan-agius4 I've done the code change directly online in GitHub and I've made a lint error (for whitespace) just fixed, but the real problems are the tests broken, can you help me with it?

@alan-agius4
Copy link
Collaborator

alan-agius4 commented Jan 20, 2020 via email

Improve paths in root tsconfig.json for better DX experience when using auto imports in IDE's.
Fix test code to conform new behaviour.

Closes angular#16709
@dmorosinotto
Copy link
Contributor Author

dmorosinotto commented Jan 20, 2020

@alan-agius4 no problem, I've done some further investigations looking and the Detail log and found the failing tests code.
I've changed it according to the new behaviour, hoping to have done it right, please check my changes , and let's me know!

Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

Caretaker note: kindly squash the commits.

@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jan 23, 2020
@kyliau kyliau merged commit 3aacf41 into angular:master Jan 24, 2020
kyliau pushed a commit that referenced this pull request Jan 24, 2020
* fix(@schematics/angular): regression tsconfig.json

fix(@schematics/angular): regression tsconfig.json  …
Unverified
0e318ae
Regression in tsconfig.json set `"outDir": "./dist/out-tsc"` for problems in VSCode TS(2307) when building library referred in tsconfig "paths"

Closes: #16708

* fix(@schematics/angular): regression tsconfig.json

Improve paths in root tsconfig.json for better DX experience when using auto imports in IDE's.

Closes #16709

* fix(@schematics/angular): regression tsconfig.json

Improve paths in root tsconfig.json for better DX experience when using auto imports in IDE's.
Fix code lint.

Closes #16709

* fix(@schematics/angular): regression tsconfig.json

Improve paths in root tsconfig.json for better DX experience when using auto imports in IDE's.
Fix test code to conform new behaviour.

Closes #16709

(cherry picked from commit 3aacf41)
@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 Feb 24, 2020
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 target: patch This PR is targeted for the next patch release
Projects
None yet
4 participants