Add a TypeScript test and fix compiler errors #1903
Conversation
There was a problem hiding this comment.
Good job, Can you also try to remove https://github.com/rollup/rollup/blob/master/tsconfig.json#L4
This will remove the possibility of errors in future when using default imports when indeed a module doesn't have
|
Great idea of adding the test :) |
Good point, I will try it – the tests are failing right now without a compiler error, need to find the culprit...
Thanks. It won't catch everything though. Especially missing dependencies won't be caught, because they are in |
|
The culprit could be any of those modules: https://github.com/rollup/rollup/blob/master/typings/declarations.d.ts#L9-L19 |
|
I definitely agree with you! I usually tend to write typings as much as I
can.
…On 23 January 2018 at 21:34, Hendrik Liebau ***@***.***> wrote:
The culprit could be any of those modules: https://github.com/rollup/
rollup/blob/master/typings/declarations.d.ts#L9-L19
By the way, I would strongly advise against defining these stubs. Ofen
times it's not that much effort to write the typings for the smaller
libraries. And for some of them even @types typings exist.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1903 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AQv-WoEurEB77UDGD_0qVxG5TAeR2A3fks5tNkJtgaJpZM4RpWsj>
.
|
|
@alan-agius4 I don't think I can make rollup work with |
|
@alan-agius4 Confirmed, so that won't work with |
|
Thanks for the try anyways :)
…On Tue, 23 Jan 2018 at 22:52, Hendrik Liebau ***@***.***> wrote:
@alan-agius4 <https://github.com/alan-agius4> Confirmed
<rollup/rollup-plugin-typescript#80>, so that
won't work with rollup-plugin-typescript. But theses type of issues would
now be covered by the test I added.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1903 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AQv-WpKUpmGb9EcY1ySTi-u-7sMbrneAks5tNlSogaJpZM4RpWsj>
.
|
|
cc: @lukastaegert |
package.json
Outdated
| "pretest-coverage": "npm run build", | ||
| "test-coverage": "rm -rf coverage/* && istanbul cover --report json node_modules/.bin/_mocha -- -u exports -R spec test/test.js", | ||
| "posttest-coverage": "remap-istanbul -i coverage/coverage-final.json -o coverage/coverage-remapped.json -b dist && remap-istanbul -i coverage/coverage-final.json -o coverage/coverage-remapped.lcov -t lcovonly -b dist && remap-istanbul -i coverage/coverage-final.json -o coverage/coverage-remapped -t html -b dist", | ||
| "ci": "npm run test-coverage && codecov < coverage/coverage-remapped.lcov", | ||
| "build": "git rev-parse HEAD > .commithash && rollup -c && tsc -p tsconfig-types.json && chmod a+x bin/rollup", | ||
| "watch": "rollup -cw", | ||
| "prepublishOnly": "npm run lint && npm run test:only && npm run test:leak", | ||
| "prepublishOnly": "npm run lint && npm run test:only && npm run test:leak && npm run test:typescript", |
There was a problem hiding this comment.
Answering your question when this test should run: As it appears that, at least on my system, this test is rather fast, I think it would be ok to include it in the test script. Otherwise the person publishing it would be the only one fixing issues here (i.e. probably me).
However I would add it after the regular test, i.e. npm run test:only && npm run test:typescript. Adding it to test will also make it run on CI. The ci script, on the other hand, seems to be unused at the moment.
Another issue I stumble upon is that the typings in the dist folder are never cleaned up which resulted in a lot of weird errors for me when running this test. My suggestion is to remove the dist/typings folder (or maybe even the dist folder) as part of the build script.
There was a problem hiding this comment.
Fixed in 65a713d. I'm not a windows guy so I'm not sure whether commands like rm or mv are still a cross-platform problem these days. Since rm -rf is already used in the test-coveragescript I used the same in prebuild. If that's a concern we could use rimraf instead. I'm not sure about the mv though. Maybe cash-mv is an option?
There was a problem hiding this comment.
Alright, appveyor just answered my question. Will fix...
- add test:typescript script to test script - remove dist directory before build
|
👍 |
fixes #1902
Open question: Should
npm run test:typescriptbe included innpm testor is it sufficient to be included inprepublishOnly? Maybe it should also be included innpm run ci?