Skip to content
This repository was archived by the owner on Apr 4, 2025. It is now read-only.

feat(@angular-devkit/build-ng-packagr): permit to set a custom TsConfig file #750

Merged
merged 3 commits into from
Apr 20, 2018
Merged

feat(@angular-devkit/build-ng-packagr): permit to set a custom TsConfig file #750

merged 3 commits into from
Apr 20, 2018

Conversation

nweldev
Copy link
Contributor

@nweldev nweldev commented Apr 20, 2018

Add a new tsConfig option in order to permit to use a custom tsConfig file when using @angular-devkit/build-ng-packagr as a build target in angular-cli.

This is, for example, needed in order to resolve this kind of issue :

$ ng build -- --project myLib
BUILD ERROR
projects/common/src/lib/foo/bar.directive.ts(134,41): error TS2339: Property 'includes' does not exist on type 'string[]'.

See the evolution of tests for an example. This example show how to change the ES level in lib and target
All the other options are required for now if we don't want to break the build. I will remove them and bump the ng-packagr dependency version in another PR once / if ng-packagr/ng-packagr#786 is released

No breaking change.

nlm-pro added 3 commits April 20, 2018 13:28
permit to use a custom tsconfig.json file by setting its path with a new tsConfig option
ngPackagr may leave some .ng_pkg_build files after an error
we may encounter this kind of errors when contributing to build_ng_packagr
Use the new tsConfig option in order to set a custom tsconfig file for the 'work' test
this example show how to change the ES level in lib and target
all the other options are required for now if we don't want to break the build
their should be removed once / if ng-packagr/ng-packagr#786 is released
@nweldev nweldev requested a review from hansl as a code owner April 20, 2018 14:31
@clydin
Copy link
Member

clydin commented Apr 20, 2018

The flaking test (css rebuilds) already has a 2 minute timeout.

@nweldev
Copy link
Contributor Author

nweldev commented Apr 20, 2018

I pushed the last git "build" commit just in order to test if it works. Will move it to another PR if ok.

@nweldev
Copy link
Contributor Author

nweldev commented Apr 20, 2018

@clydin oh, ok, sorry I didn't saw that. Is there something I could do then ? Do you think some of my code break the tests on appveyor for some reason ?

@clydin
Copy link
Member

clydin commented Apr 20, 2018

It's not your code. It appears to be a resource issue with AppVeyor. As long as it is only that one particular test, you can ignore it for now.

@filipesilva
Copy link
Contributor

Oh this is really nice, I didn't know ng-packagr supported a custom tsconfig already!

I'm a bit worried about the mandatory options but that seems like it can go away in the future.

Also wondering if we should also generate a tsconfig when making a library. That would avoid having the linting tsconfig which kinda bothered me as well. WDYT?

@nweldev
Copy link
Contributor Author

nweldev commented Apr 20, 2018

@clydin Ok so it's non of my business, I'll just delete this commit then ^^ I was worried that the ❌ could make you think that some PRs doesn't deserve review 😄

const ngPkgProject = projectNgPackagr.ngPackagr()
.forProject(packageJsonPath);

if (options.tsConfig) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@filipesilva (about your first concern) except if I didn't tought about something, the tsConfig is not mandatory (you can see that the tests doesn't fail after the first commit), given that I first test if it's set before using it in the ngPackagr. Did I forgot some "optionnal" config somewhere ?

@nweldev
Copy link
Contributor Author

nweldev commented Apr 20, 2018

@filipesilva (about your second concern) I don't think that adding a tsConfig by default would be a good choice, given that ngPackagr do a lot of custom things with it. For now, there is still some issues with that (see ng-packagr/ng-packagr#786) so I don't think it's recommanded to use custom tsConfig except you really need it (like in the example in the tests). @dherges ?

FYI : I was already thinking about adding a --tsconfig option to the library schematics with a next PR, but it would realy just be "cherry on the cake", so not a priority

@nweldev
Copy link
Contributor Author

nweldev commented Apr 20, 2018

and also @filipesilva "Oh this is really nice" 🎉 thanks a lot

@filipesilva
Copy link
Contributor

When I mentioned the mandatory options I mean the ones inside the tsconfig (as per the issue you linked), not in the builder here. The option you added is optional and stuff works if we don't add a tsconfig.

So yeah, I think what you added in this PR is ideal for now. Later maybe we can reconsider the tsconfig in a newly generated library.

@filipesilva
Copy link
Contributor

Thanks for adding this option!

@filipesilva filipesilva merged commit 766059b into angular:master Apr 20, 2018
@dherges
Copy link
Contributor

dherges commented Apr 21, 2018

Some additional info / clarifications: therea re several places in ng-packagr where a tsconfig is generated. Summarized:

  • a default tsconfig (or base config). this one is customizable
  • a tsconfig generated per entry point (the compilation unit that is built). auto-generated, not customizable
  • finally, a tsconfig for compilation of the entry point (specific for the compilation target, e.g. es5 and es2015). auto-generated and not customizable.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants