Skip to content

build(bazel): allow a user to control a subset of angularCompilerOption in their tsconfig file when using ng_module - #28995

Closed
gregmagolan wants to merge 2 commits into
angular:masterfrom
gregmagolan:build/allow-user-angular-compiler-options
Closed

build(bazel): allow a user to control a subset of angularCompilerOption in their tsconfig file when using ng_module#28995
gregmagolan wants to merge 2 commits into
angular:masterfrom
gregmagolan:build/allow-user-angular-compiler-options

Conversation

@gregmagolan

@gregmagolan gregmagolan commented Feb 26, 2019

Copy link
Copy Markdown
Contributor

Unblocks #28689 which requires the use of

  "angularCompilerOptions": {
    "i18nInFile": "translations.xlf"
  }

@gregmagolan
build(bazel): allow a user to control a subset of angularCompilerOpti…
59a3520 
…on in their tsconfig file when using ng_module
@gregmagolan
gregmagolan requested review from a team and alexeagle February 26, 2019 23:37
@alexeagle alexeagle added the target: patch This PR is targeted for the next patch release label Feb 26, 2019
@gregmagolan gregmagolan added the action: merge The PR is ready for merge by the caretaker label Feb 26, 2019
@benlesh benlesh added area: build & ci Related the build and CI infrastructure of the project area: bazel Issues related to the published `@angular/bazel` build rules labels Feb 27, 2019
@ngbot ngbot Bot modified the milestone: needsTriage Feb 27, 2019
alan-agius4
alan-agius4 reviewed Feb 27, 2019
View reviewed changes
Comment thread packages/bazel/src/ngc-wrapped/index.ts
// Allow Bazel users to control some of the bazel options.
// Since TypeScript's "extends" mechanism applies only to "compilerOptions"
// we have to repeat some of their logic to get the user's "angularCompilerOptions".
if (config['extends']) {

@alan-agius4 alan-agius4 Feb 27, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

extends can be available at multiple levels.
See: https://github.com/bazelbuild/rules_typescript/pull/265/files#diff-8c2aa63f0643c2daf20032369cf0a48fR286 & https://github.com/bazelbuild/rules_typescript/pull/265/files#diff-052e72d7fea048c0c20120cdda7337bdR61

@ocombe ocombe Feb 27, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah in my case with this code it gets: /home/ocombe/.cache/bazel/_bazel_ocombe/df516b31b5c55b244dfb17f03a952d3d/execroot/angular/packages/tsconfig-build.json but it doesn't read the local tsconfig file that I defined in my bundle test folder

ocombe
ocombe reviewed Feb 27, 2019
View reviewed changes
Comment thread packages/bazel/src/ngc-wrapped/index.ts

// All user angularCompilerOptions values that a user has control
// over should be collected here
if (userConfig.angularCompilerOptions) {

@ocombe ocombe Feb 27, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can't you make a loop that gets all properties of userConfig? We wouldn't need to maintain this when we add new compiler config options. And worst case scenario it would set an option that wouldn't be used

@alexeagle alexeagle Feb 27, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

there are some properties which bazel needs to control, I think flatModuleId is an example.

ngc-wrapped is planned to be deprecated with Ivy so we don't need to put that much care here about maintainability

benlesh pushed a commit that referenced this pull request Feb 27, 2019
@gregmagolan @benlesh
build(bazel): allow a user to control a subset of angularCompilerOpti…
09fc669 
…on in their tsconfig file when using ng_module (#28995)

PR Close #28995
benlesh pushed a commit that referenced this pull request Feb 27, 2019
@gregmagolan @benlesh
style: format (#28995)
feec963 
PR Close #28995
@benlesh benlesh closed this in c4c3c12 Feb 27, 2019
benlesh pushed a commit that referenced this pull request Feb 27, 2019
@gregmagolan @benlesh
style: format (#28995)
7b944c4 
PR Close #28995
@ocombe

ocombe commented Feb 27, 2019

Copy link
Copy Markdown
Contributor

@benlesh I don't think it was ready for merge, @alan-agius4 seems to have a legitimate comment, and the PR doesn't seem to fix my issue (which was the goal)
it's probably not your fault since there was a merge label :(
I guess there should be a follow up PR then

@gregmagolan

gregmagolan commented Feb 27, 2019

Copy link
Copy Markdown
Contributor Author

@benlesh I don't think it was ready for merge, @alan-agius4 seems to have a legitimate comment, and the PR doesn't seem to fix my issue (which was the goal)
it's probably not your fault since there was a merge label :(
I guess there should be a follow up PR then

@ocombe I tried the failure target with this change and it fixed it locally. Are you still failing on the same issue?

@eachirei

eachirei commented Feb 27, 2019

Copy link
Copy Markdown

Sorry to hop in, but couldn't this also include

{
    "skipTemplateCodegen": true,
    "strictMetadataEmit": true,
    "fullTemplateTypeCheck": true
  }

I got some issues when building my project regarding

Error during template compile of 'AppModule'
  Function calls are not supported in decorators but 'StoreModule' was called.

and I understand that these options could fix that issue.

@gregmagolan

gregmagolan commented Feb 27, 2019

Copy link
Copy Markdown
Contributor Author

@eachirei fullTemplateTypeCheck should default to true (you can also control that one with the type_check attribute on ng_module. The other two may be controllable under Bazel but I'm not sure of the implications of controlling metadata emit under Bazel which is why I left them out from the PR.

Have you looked at #23609?

@eachirei

eachirei commented Feb 27, 2019

Copy link
Copy Markdown

I'll give it another shot and try the method described in latest comment on that issue.

@eachirei

eachirei commented Feb 27, 2019

Copy link
Copy Markdown

Thing is the modules that are causing this problem are from external libraries, so I'll try to wrap them up with that method. I do not know the real cause of this, but I don't think this approach is feasible though.

@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 14, 2019

Copy link
Copy Markdown

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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@alan-agius4 alan-agius4 alan-agius4 left review comments
+2 more reviewers
@alexeagle alexeagle alexeagle approved these changes
@ocombe ocombe ocombe left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: bazel Issues related to the published `@angular/bazel` build rules area: build & ci Related the build and CI infrastructure of the project cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

needsTriage

Development

Successfully merging this pull request may close these issues.

7 participants

@gregmagolan @ocombe @eachirei @alexeagle @alan-agius4 @benlesh @googlebot