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(webpack): fix various TypeScript typing error #1091

Merged
merged 3 commits into from
May 2, 2019
Merged

fix(webpack): fix various TypeScript typing error #1091

merged 3 commits into from
May 2, 2019

Conversation

3cp
Copy link
Member

@3cp 3cp commented Apr 24, 2019

No description provided.

@3cp
Copy link
Member Author

3cp commented Apr 24, 2019

Manually tested on few selected skeletons but not all.

@Sayan751
Copy link
Member

@huochunpeng Unfortunately, I could not replicate the issue #1090. I have following package versions for my project.

protractor in my package-lock does not have explicit dependencies on @types/q, which completely avoids the problem with duplicated typings. I think a mere update of the packages (removing both node_modules and package-lock, if permitted), should correct the typings.

It might not be the case, but I think fixing the typeroot in this way may cause problem in a monorepo setup with hoisted dependency installation. From TS documentation:

By default all visible @types packages are included in your compilation. Packages in node_modules/@types of any enclosing folder are considered visible; specifically, that means packages within ./node_modules/@types/, ../node_modules/@types/, ../../node_modules/@types/, and so on.

@MichaelPetrinolis
Copy link
Contributor

Did you try to create a new project? With latest vscode, node and Aurelia cli, on two different boxes, I can reproduce it.

@Sayan751
Copy link
Member

I created a new project using [email protected], latest vscode, node v10.15.3, npm v6.9.0.

@3cp
Copy link
Member Author

3cp commented Apr 29, 2019

I am seeing different type issue on the default typescript app.

cli v1.0.0-beta.15
node v10.15.3
npm v6.9.0
installed typescript v3.4.5

⋊> ~/p/w au run --open                                                                                                                                                      
Local aurelia-cli v1.0.0-beta.15
Starting 'configureEnvironment'...
Finished 'configureEnvironment'
Starting 'runWebpack'...
Project is running at http://localhost:8080
webpack output is served from /
Content not from webpack is served from /Users/huocp/playground/w/dist
404s will fallback to /index.html
Finished 'runWebpack'
ℹ 「wdm」: wait until bundle finished: /
ℹ 「wdm」: wait until bundle finished: /af7ae505a9eed503f8b8e6982036873e.woff2
✖ 「wdm」: Hash: f8f9cd36d11264591384
Version: webpack 4.30.0
Time: 5510ms
Built at: 04/29/2019 10:50:16 AM
                                         Asset      Size            Chunks                    Chunk Names
    app~d0ae3f07.f8f9cd36d11264591384.chunk.js  9.49 KiB      app~d0ae3f07  [emitted]         app~d0ae3f07
                                   favicon.ico  14.7 KiB                    [emitted]         
                                    index.html  1.87 KiB                    [emitted]         
    runtime~app.f8f9cd36d11264591384.bundle.js  6.05 KiB       runtime~app  [emitted]         runtime~app
vendors~02227409.f8f9cd36d11264591384.chunk.js   472 KiB  vendors~02227409  [emitted]  [big]  vendors~02227409
vendors~11bb84b7.f8f9cd36d11264591384.chunk.js   127 KiB  vendors~11bb84b7  [emitted]         vendors~11bb84b7
vendors~1f20a385.f8f9cd36d11264591384.chunk.js   178 KiB  vendors~1f20a385  [emitted]         vendors~1f20a385
vendors~3667cd95.f8f9cd36d11264591384.chunk.js   477 KiB  vendors~3667cd95  [emitted]  [big]  vendors~3667cd95
vendors~41983590.f8f9cd36d11264591384.chunk.js   453 KiB  vendors~41983590  [emitted]  [big]  vendors~41983590
vendors~50e8d500.f8f9cd36d11264591384.chunk.js   272 KiB  vendors~50e8d500  [emitted]  [big]  vendors~50e8d500
vendors~556c66f2.f8f9cd36d11264591384.chunk.js   259 KiB  vendors~556c66f2  [emitted]  [big]  vendors~556c66f2
vendors~5ea1390f.f8f9cd36d11264591384.chunk.js  43.2 KiB  vendors~5ea1390f  [emitted]         vendors~5ea1390f
vendors~60fca16e.f8f9cd36d11264591384.chunk.js  55.4 KiB  vendors~60fca16e  [emitted]         vendors~60fca16e
vendors~72fdf3f2.f8f9cd36d11264591384.chunk.js  72.6 KiB  vendors~72fdf3f2  [emitted]         vendors~72fdf3f2
vendors~7e79ec10.f8f9cd36d11264591384.chunk.js   495 KiB  vendors~7e79ec10  [emitted]  [big]  vendors~7e79ec10
vendors~987e6011.f8f9cd36d11264591384.chunk.js   533 KiB  vendors~987e6011  [emitted]  [big]  vendors~987e6011
vendors~b55f28a1.f8f9cd36d11264591384.chunk.js  35.6 KiB  vendors~b55f28a1  [emitted]         vendors~b55f28a1
vendors~cfbf0a2e.f8f9cd36d11264591384.chunk.js   131 KiB  vendors~cfbf0a2e  [emitted]         vendors~cfbf0a2e
vendors~ecff2e3d.f8f9cd36d11264591384.chunk.js   364 KiB  vendors~ecff2e3d  [emitted]  [big]  vendors~ecff2e3d
Entrypoint app [big] = runtime~app.f8f9cd36d11264591384.bundle.js vendors~7e79ec10.f8f9cd36d11264591384.chunk.js vendors~556c66f2.f8f9cd36d11264591384.chunk.js vendors~72fdf3f2.f8f9cd36d11264591384.chunk.js vendors~50e8d500.f8f9cd36d11264591384.chunk.js vendors~5ea1390f.f8f9cd36d11264591384.chunk.js vendors~ecff2e3d.f8f9cd36d11264591384.chunk.js vendors~02227409.f8f9cd36d11264591384.chunk.js vendors~41983590.f8f9cd36d11264591384.chunk.js vendors~987e6011.f8f9cd36d11264591384.chunk.js vendors~11bb84b7.f8f9cd36d11264591384.chunk.js vendors~b55f28a1.f8f9cd36d11264591384.chunk.js vendors~1f20a385.f8f9cd36d11264591384.chunk.js vendors~60fca16e.f8f9cd36d11264591384.chunk.js vendors~3667cd95.f8f9cd36d11264591384.chunk.js vendors~cfbf0a2e.f8f9cd36d11264591384.chunk.js app~d0ae3f07.f8f9cd36d11264591384.chunk.js
[0] multi (webpack)-dev-server/client?http://localhost:8080 aurelia-webpack-plugin/runtime/empty-entry aurelia-webpack-plugin/runtime/pal-loader-entry webpack-dev-server/client aurelia-bootstrapper 76 bytes {app~d0ae3f07} [built]
[+gfj] (webpack)-dev-server/client/overlay.js 3.59 KiB {vendors~cfbf0a2e} [built]
[5jyU] ./node_modules/aurelia-loader-webpack/dist/native-modules/aurelia-loader-webpack.js 15.2 KiB {vendors~556c66f2} [built]
[70NS] ./node_modules/aurelia-pal/dist/native-modules/aurelia-pal.js 2.18 KiB {vendors~72fdf3f2} [built]
[8oxB] ./node_modules/process/browser.js 5.29 KiB {vendors~60fca16e} [built]
[CxY0] ./node_modules/url/url.js 22.8 KiB {vendors~cfbf0a2e} [built]
[EpB7] ./node_modules/node-libs-browser/node_modules/querystring-es3/index.js 127 bytes {vendors~1f20a385} [built]
[GAND] ./node_modules/aurelia-webpack-plugin/runtime/empty-entry.js 585 bytes {vendors~5ea1390f} [built]
[GBRk] (webpack)-dev-server/client?http://localhost:8080 8.26 KiB {vendors~cfbf0a2e} [built]
[GmYv] ./node_modules/aurelia-webpack-plugin/runtime/pal-loader-entry.js 1.56 KiB {vendors~5ea1390f} [built]
[K+kI] (webpack)-dev-server/client/index.js 8.26 KiB {vendors~cfbf0a2e} [built]
[Q/5p] ./node_modules/aurelia-polyfills/dist/native-modules/aurelia-polyfills.js 24.2 KiB {vendors~50e8d500} [built]
[R6Jb] (webpack)-dev-server/client/socket.js 1.05 KiB {vendors~cfbf0a2e} [built]
[XH0y] ./node_modules/loglevel/lib/loglevel.js 7.68 KiB {vendors~1f20a385} [built]
[b9nV] ./node_modules/aurelia-bootstrapper/dist/native-modules/aurelia-bootstrapper.js 5.51 KiB {vendors~7e79ec10} [built]
    + 442 hidden modules

ERROR in /Users/huocp/playground/w/test/unit/app.spec.ts
[tsl] ERROR in /Users/huocp/playground/w/test/unit/app.spec.ts(5,1)
      TS2582: Cannot find name 'describe'. Do you need to install type definitions for a test runner? Try `npm i @types/jest` or `npm i @types/mocha`.

ERROR in /Users/huocp/playground/w/test/unit/app.spec.ts
[tsl] ERROR in /Users/huocp/playground/w/test/unit/app.spec.ts(8,3)
      TS2304: Cannot find name 'beforeEach'.

ERROR in /Users/huocp/playground/w/test/unit/app.spec.ts
[tsl] ERROR in /Users/huocp/playground/w/test/unit/app.spec.ts(14,3)
      TS2304: Cannot find name 'afterEach'.

ERROR in /Users/huocp/playground/w/test/unit/app.spec.ts
[tsl] ERROR in /Users/huocp/playground/w/test/unit/app.spec.ts(16,3)
      TS2582: Cannot find name 'it'. Do you need to install type definitions for a test runner? Try `npm i @types/jest` or `npm i @types/mocha`.

ERROR in /Users/huocp/playground/w/test/unit/app.spec.ts
[tsl] ERROR in /Users/huocp/playground/w/test/unit/app.spec.ts(19,7)
      TS2304: Cannot find name 'expect'.

ERROR in /Users/huocp/playground/w/test/unit/app.spec.ts
[tsl] ERROR in /Users/huocp/playground/w/test/unit/app.spec.ts(22,7)
      TS2304: Cannot find name 'fail'.
Child html-webpack-plugin for "index.html":
         Asset      Size  Chunks  Chunk Names
    index.html  1.36 MiB       0  
    Entrypoint undefined = index.html
    [8XHo] ./node_modules/html-webpack-plugin/lib/loader.js!./index.ejs 627 bytes {0} [built]
    [LvDl] ./node_modules/lodash/lodash.js 527 KiB {0} [built]
    [YuTi] (webpack)/buildin/module.js 497 bytes {0} [built]
    [yLpj] (webpack)/buildin/global.js 472 bytes {0} [built]
ℹ 「wdm」: Failed to compile.

@3cp
Copy link
Member Author

3cp commented Apr 29, 2019

@bigopon @EisenbergEffect question: do you know why aurelia-loader-nodejs has a reference to @types/lodash? I checked its package-lock.json, there is no lodash usage at all for any dependencies.

https://github.com/aurelia/loader-nodejs/blob/7bfce9a1a4d4996c84dd6888e911a8aad17f8ef3/dist/commonjs/aurelia-loader-nodejs.d.ts#L1

@bigopon
Copy link
Member

bigopon commented Apr 29, 2019

Probably unintended dep. Can just remove it

@3cp
Copy link
Member Author

3cp commented Apr 29, 2019

Is it simply removing @types/lodash from package.json?

@bigopon can you clean up TS syntax? The project doesn't compile with latest TS v2, probably need to upgrade to TS v3 too.

@Sayan751
Copy link
Member

@huochunpeng I am also seeing the similar typings errors in the default TS app as pointed by you. However, I am wondering, what is the need for compiling the spec files for running/building the app. Those files can be built while testing the app, if I am not missing anything. Therefore, I have changed the ts-loader usage to following to get rid of the superfluous error messages.

{ test: /\.ts$/, loader: [{ loader: "ts-loader", options: { reportFiles: [ srcDir+'/**/*.ts'] } }], include: srcDir }

However, in the default app I see dependency neither on q nor on protractor.

@3cp 3cp changed the title fix(skeleton): fix some TS typing error by fixing typeRoots fix(webpack): fix TypeScript error on unneeded test code when running webpack Apr 30, 2019
@3cp
Copy link
Member Author

3cp commented Apr 30, 2019

I have revised this PR:

  1. reverted the removal of @types/lodash, which has to wait for Unnecessary reference to @types/lodash loader-nodejs#5.
  2. not using typeRoot, but updated webpack ts-loader option to skip test folder. Thanks @Sayan751 for the tip!

@3cp
Copy link
Member Author

3cp commented Apr 30, 2019

@MichaelPetrinolis could you create a demo repo with package-lock.json (or yarn.lock) committed for us to reproduce your issue?

@MichaelPetrinolis
Copy link
Contributor

Thank you, i create a new project with the following command
au new testapp -u -s http2,typescript,htmlmin-max,sass,postcss-typical,karma,protractor,vscode
This zipped file is the created project without npm install testapp.zip

then npm install, au run and have the following error
ERROR in C:\Users\micha\source\repos\testapp\tsconfig.json
[tsl] ERROR
TS4090: Conflicting definitions for 'q' found at 'C:/Users/micha/source/repos/testapp/node_modules/@types/q/index.d.ts' and 'C:/Users/micha/source/repos/testapp/node_modules/protractor/node_modules/@types/q/index.d.ts'. Consider installing a specific version of this library to resolve the conflict.

Here is the created file after npm i
package-lock.zip

@3cp
Copy link
Member Author

3cp commented May 1, 2019

@MichaelPetrinolis thx! The conflicting @types/q is from cssnano (by selecting postcss-typical).

⋊> ~/p/testapp npm list @types/q
[email protected] /Users/huocp/playground/testapp
├─┬ [email protected]
│ └─┬ [email protected]
│   └─┬ [email protected]
│     └─┬ [email protected]
│       └─┬ [email protected]
│         └── @types/[email protected] 
└─┬ [email protected]
  └── @types/[email protected] 

The fix from @Sayan751 doesn't help your case because that only deals with errors on test/ folder.

@MichaelPetrinolis
Copy link
Contributor

using typesRoot fixes it. But don't know if it will have side effects.

@3cp
Copy link
Member Author

3cp commented May 1, 2019

As @Sayan751 pointed out, typeRoot has a small problem that doesn’t work for user who uses lerna.

Since it’s between cssnano and protractor for typescript, I can conditionally add typeRoot for the combination, so that the typeRoot’s problem on lerna would not affect most users. Will update this PR.

… protractor are selected

cssnano and protractor load different version of @types/q which confused TypeScript. It's likely only confusing ts-loader because only webpack app is affected, not cli-bundler app.
Setting typeRoots is not flexible in lerna hoisting environment, that's why we did not turn it globally for webpack+TypeScript app.

closes #1090
@3cp
Copy link
Member Author

3cp commented May 1, 2019

@Sayan751 can you do another review?

@3cp 3cp changed the title fix(webpack): fix TypeScript error on unneeded test code when running webpack fix(webpack): fix various TypeScript typing error May 1, 2019
@3cp
Copy link
Member Author

3cp commented May 1, 2019

For reference, protractor next version v6.0.0 (but v5.4.3 is still latest on npmjs.com) has removed q and @types/q from dependencies.
Our skeleton e2e still works with protractor v6. We will update to v6 when it's marked at latest on npmjs.

@Sayan751
Copy link
Member

Sayan751 commented May 2, 2019

The changes look good to me.

Just as a side note, maybe a new issue needs to be opened to track the task of updating protractor version to 6.x when it is made latest in npm.

@3cp
Copy link
Member Author

3cp commented May 2, 2019

#1097

@3cp
Copy link
Member Author

3cp commented May 2, 2019

@EisenbergEffect can be merged. hold for a while, forgot to test webpack+karma setup

@Sayan751
Copy link
Member

Sayan751 commented May 2, 2019

Sorry to note this late, but has anyone run the tests with new webpack config? As the config is changed, does that affect the unit tests?

@Sayan751
Copy link
Member

Sayan751 commented May 2, 2019

I have 2 different configurations for tests and dev that looks something like below.

...when(isTest,
  [
    { test: /\.ts$/i, 
      use:[
        //this is just for code-coverage and can be ignored for general setup.
        { loader: "istanbul-instrumenter-loader" },
        { loader: "ts-loader", options: { reportFiles: [ srcDir+'/**/*.ts'] } }
      ], 
      include: srcDir },
    { test: /\.ts$/i, loader: "ts-loader", include: testDir, options: { reportFiles: [testDir+'/**/*.ts'] } }
  ],
  [ { test: /\.ts$/i, loader: [{ loader: "ts-loader", options: { reportFiles: [ srcDir+'/**/*.ts'] } }], include: srcDir } ])

@3cp
Copy link
Member Author

3cp commented May 2, 2019

@Sayan751 I was able to run karma for webpack+typescript+karma app without additional fix on the ts-loader. I have no idea why the loader: "ts-loader", options: { reportFiles: [ srcDir+'/**/*.ts'] }, include: srcDir has no impact on loading test/ unit test files.

@Sayan751
Copy link
Member

Sayan751 commented May 2, 2019

@huochunpeng At least it is now known that the changed config does not affect the tests. 🙂 I think that is it. I cannot see any other immediate issue with the changes 👍

@3cp
Copy link
Member Author

3cp commented May 2, 2019

Thx.

@EisenbergEffect let's merge this.

@EisenbergEffect EisenbergEffect merged commit 1e6c1dc into aurelia:master May 2, 2019
@3cp 3cp deleted the cleanup-ts branch May 27, 2019 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants