Use application tsconfig file for indexTransform and customWebpackConfig #879

michelherv · 2020-11-18T01:19:08Z

PR Checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

The application's tsconfig is ignore. Accordingly, indexTransform and customWebpackConfig cannot use project configuration for custom modules for example

Issue Number: #607 #749

What is the new behavior?

The application's tsconfig is now use. indexTransform and customWebpackConfig can now use all the application resources.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

just-jeb · 2020-11-19T10:23:04Z

Hi. First of all thanks for your contribution - it is highly appreciated!
Your PR in some way intersects with #824, and the reason the build is failing is the same reason.
If you have time and desire to investigate this issue, it will be incredibly helpful.

just-jeb · 2020-11-26T11:43:39Z

Hey, thanks for the contribution.
Currently two things are missing here - updating e2e tests and docs.
I'd start with e2e, since I have a strong suspicion they will start failing once you try to use .ts config with Karma builder like here.

scripts/run-ci.sh

muuvmuuv · 2020-12-09T06:46:36Z

packages/custom-webpack/package.json

@@ -42,6 +42,7 @@
    "@angular-devkit/core": "^11.0.0",
    "lodash": "^4.17.15",
    "ts-node": "^9.0.0",
+    "tsconfig-paths": "^3.9.0",


I am currently merging part of your changes into my PR (#824) and adding tsconfig-paths was not needed. Why are you adding it, are there any benefits?

As far as I remember ts-node does not support paths property, so if you want to have the exact same config (including paths) for your webpack (or index) configuration you need this one.

Makes sense! But ts-node isn't needed for any Angular project since it compiles through tsc and does not require to be "started" through Node, so I wouldn't add this or do I miss any usecase here?

ts-node is indeed not needed for Angular app, because Angular compiler (not exactly tsc) builds it, that's correct.
However, index transform and webpack config are not part of the app, they are part of the build chain, which in our case is custom-webpack builder. The builder needs ts-node in order to understand the TS syntax of the files it loads (webpack config and index transform).

michelherv · 2020-12-12T22:08:56Z

Hi,
I updated e2e tests. Can you read the pull request again and tell me if tests are ok for you. Else can you help me by pushing some tests? I will try to improve the solution by TDD.

About the documentation, I have no improvement idear. tsConfig is a @angular-devkit/architect:browser property. It's not like customWebpackConfig or indexTransform which are custom properties. I don't know what to add about.

just-jeb · 2020-12-14T13:34:13Z

@muuvmuuv @michelherv I appreciate the effort and thank you both, but currently we have two PRs that do practically the same. Can we please have a single PR that takes the best of the existing PRs?
Would be much easier for me to follow and comment.
Thank you!

muuvmuuv · 2020-12-14T14:56:12Z

@michelherv @just-jeb before new year I won't have time for any OSS, so I would close mine and Michel can include all parts you both think that are missing here.

Below or things I would change or add, doesn't mean they should be included in this PR.

workspace npm scripts should use binaries directly instead of npx: https://github.com/just-jeb/angular-builders/pull/824/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R15
puppeteer for karma testing browser bin to avoid using none-existing local binary: https://github.com/just-jeb/angular-builders/pull/824/files#diff-5948cfd5023315c8bda008c2c6688e5a232dd1e0e4932ccfa01db65743069077R3
use karma-coverage: https://github.com/just-jeb/angular-builders/pull/824/files#diff-5948cfd5023315c8bda008c2c6688e5a232dd1e0e4932ccfa01db65743069077R18
just use the version without © for better test parsing: https://github.com/just-jeb/angular-builders/pull/824/files#diff-f91984e0f1388cbc9fd6c3fb6ba9dbe36b6b08425104d2af479e64ef4b38dfd7R25
add some note why we use the node-typing: https://github.com/just-jeb/angular-builders/pull/824/files#diff-a5e05c6f6ee63c72a42f6f6662bc73589c4c7880a083f414c6441267d53694ebR6
use file import: https://github.com/just-jeb/angular-builders/pull/824/files#diff-8befd2cce9bc793e8dc30fc050dbbf77f613ef94042f12fb1dca6f1e3b746fc1R8
fix target option typing bug (appeared in my installation): https://github.com/just-jeb/angular-builders/pull/824/files#diff-b2c00b61854bfb83318b9fa5f3c5110bae9a2fba71f2e9a973059665f6d0651cR17

just-jeb · 2020-12-17T14:35:44Z

Hey, sorry it takes me so long to respond, I'm trying to make the v11 stable (#908).

Hi,
I updated e2e tests. Can you read the pull request again and tell me if tests are ok for you. Else can you help me by pushing some tests? I will try to improve the solution by TDD.

About the documentation, I have no improvement idear. tsConfig is a @angular-devkit/architect:browser property. It's not like customWebpackConfig or indexTransform which are custom properties. I don't know what to add about.

I'll take a look at tests and leave my comments as a review.
Regarding documentation; by adding docs I meant that we need to specify in the docs that indexTransform can now be written with Typescript, and that both customWebpackConfig.ts and indexTransfrom.ts are using the app tsConfig now.
Also if any changes have to be applied to tsconfig.json in order to make it work, (like "types": [ "node" ]) then we should mention it in the docs as well.

just-jeb

Looks good overall. But I think we also need a test that makes sure indexTransform uses the app config as well.

packages/custom-webpack/examples/full-cycle-app/tsconfig.base.json

.github/workflows/angular-builders.yml

packages/custom-webpack/examples/full-cycle-app/angular.json

packages/custom-webpack/examples/full-cycle-app/tsconfig.app.json

michelherv · 2021-01-19T21:45:48Z

Hi,
I have a problem with the build process

Do you understand what is the problem ?

just-jeb · 2021-01-20T13:32:47Z

Hi,
I have a problem with the build process

Do you understand what is the problem ?

Oh, I think I read something about it. As far as I remember it's because it's a fork, and you don't have a token defined in your GH secrets.
Let me try to find the issue.

just-jeb · 2021-01-20T13:34:22Z

Here it is: actions/checkout#298

just-jeb · 2021-01-20T13:36:42Z

Seems like there is a workaround. I'll try to apply it.

just-jeb · 2021-01-20T14:58:42Z

Aaaand fixed!

just-jeb · 2021-01-27T10:23:18Z

@michelherv are we good to go? Can this be merged?

michelherv · 2021-01-29T20:24:41Z

Hi @just-jeb , it is ok now for merging. Sorry for the late, I was very busy these last days.

According @muuvmuuv suggestions :

I just use the version without © for better test parsing ;
I add some note why we use the node-typing.

The others suggestions are also very important. I think we should have to treat them as real independent improvements which will be keep anyway independently of this PR. They would can be merge before or after this PR. It it why I opened the PR #925

tony · 2021-06-17T13:01:27Z

packages/custom-webpack/examples/full-cycle-app/angular.json

@@ -50,7 +53,6 @@
              "optimization": true,
              "outputHashing": "all",
              "sourceMap": false,
-              "extractCss": true,


Option "extractCss" is deprecated: Deprecated since version 11.0. No longer required to disable CSS extraction for HMR.

michelherv closed this Nov 24, 2020

michelherv mentioned this pull request Nov 24, 2020

Use client tsconfig file #885

Closed

2 tasks

michelherv reopened this Nov 25, 2020

michelherv mentioned this pull request Nov 25, 2020

Fix e2e test #886

Merged

2 tasks

just-jeb mentioned this pull request Dec 8, 2020

feat(custom-webpack): add project to ts node registration #824

Closed

2 tasks

muuvmuuv reviewed Dec 9, 2020

View reviewed changes

scripts/run-ci.sh Outdated Show resolved Hide resolved

muuvmuuv reviewed Dec 9, 2020

View reviewed changes

just-jeb force-pushed the master branch 3 times, most recently from b1c8c3a to 5d8194c Compare December 14, 2020 13:11

just-jeb reviewed Dec 17, 2020

View reviewed changes

just-jeb mentioned this pull request Jan 20, 2021

ci: allow checkout without the GH token (for forks) #921

Merged

michelherv mentioned this pull request Jan 29, 2021

Chore: workspace improvement #925

Merged

1 task

michelherv added 4 commits February 3, 2021 10:17

chore: use the application tsconfig file

596240a

chore: add tsconfig-paths for paths compiler-options

de6534e

docs: add informations about improvements

af7ef1d

fix: update examples config files

0d67172

michelherv and others added 4 commits February 3, 2021 10:17

feat: add test for the new feature

1de68ee

Merge branch 'master' into master

cb16531

Merge branch 'master' into master

f565172

Merge branch 'master' into master

867ebc0

just-jeb approved these changes Feb 3, 2021

View reviewed changes

just-jeb merged commit c85cd10 into just-jeb:master Feb 3, 2021

tony mentioned this pull request Mar 2, 2021

custom-webpack: Invalid or unexpected token @ 11.0.0 -> 11.1.0 (and 11.1.1) #944

Closed

tony reviewed Jun 17, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use application tsconfig file for indexTransform and customWebpackConfig #879

Use application tsconfig file for indexTransform and customWebpackConfig #879

michelherv commented Nov 18, 2020 •

edited

Loading

just-jeb commented Nov 19, 2020

just-jeb commented Nov 26, 2020

muuvmuuv Dec 9, 2020

just-jeb Dec 9, 2020

muuvmuuv Dec 9, 2020

just-jeb Dec 10, 2020 •

edited

Loading

michelherv commented Dec 12, 2020

just-jeb commented Dec 14, 2020

muuvmuuv commented Dec 14, 2020

just-jeb commented Dec 17, 2020

just-jeb left a comment •

edited

Loading

michelherv commented Jan 19, 2021

just-jeb commented Jan 20, 2021

just-jeb commented Jan 20, 2021

just-jeb commented Jan 20, 2021

just-jeb commented Jan 20, 2021

just-jeb commented Jan 27, 2021

michelherv commented Jan 29, 2021

tony Jun 17, 2021

Use application tsconfig file for indexTransform and customWebpackConfig #879

Use application tsconfig file for indexTransform and customWebpackConfig #879

Conversation

michelherv commented Nov 18, 2020 • edited Loading

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

just-jeb commented Nov 19, 2020

just-jeb commented Nov 26, 2020

muuvmuuv Dec 9, 2020

Choose a reason for hiding this comment

just-jeb Dec 9, 2020

Choose a reason for hiding this comment

muuvmuuv Dec 9, 2020

Choose a reason for hiding this comment

just-jeb Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

michelherv commented Dec 12, 2020

just-jeb commented Dec 14, 2020

muuvmuuv commented Dec 14, 2020

just-jeb commented Dec 17, 2020

just-jeb left a comment • edited Loading

Choose a reason for hiding this comment

michelherv commented Jan 19, 2021

just-jeb commented Jan 20, 2021

just-jeb commented Jan 20, 2021

just-jeb commented Jan 20, 2021

just-jeb commented Jan 20, 2021

just-jeb commented Jan 27, 2021

michelherv commented Jan 29, 2021

tony Jun 17, 2021

Choose a reason for hiding this comment

michelherv commented Nov 18, 2020 •

edited

Loading

just-jeb Dec 10, 2020 •

edited

Loading

just-jeb left a comment •

edited

Loading