Add a TypeScript test and fix compiler errors by unstubbable · Pull Request #1903 · rollup/rollup

unstubbable · 2018-01-23T09:45:42Z

fixes #1902

Open question: Should npm run test:typescript be included in npm test or is it sufficient to be included in prepublishOnly? Maybe it should also be included in npm run ci?

alan-agius4

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

alan-agius4 · 2018-01-23T20:27:31Z

Great idea of adding the test :)

unstubbable · 2018-01-23T20:31:26Z

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

Good point, I will try it – the tests are failing right now without a compiler error, need to find the culprit...

Great idea of adding the test :)

Thanks. It won't catch everything though. Especially missing dependencies won't be caught, because they are in node_modules.

unstubbable · 2018-01-23T20:34:47Z

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. Often times it's not that much effort to write the typings for the smaller libraries. And for some of them even @types typings exist. (EDIT: Well, it was only one: pretty-ms.)

alan-agius4 · 2018-01-23T20:36:32Z

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> .

unstubbable · 2018-01-23T21:44:06Z

@alan-agius4 I don't think I can make rollup work with allowSyntheticDefaultImports set to false. Here is what I've got so far. But with that change suddenly any module in node_modules can not require path anymore, for example!? I'm not sure whether this is a constraint of rollup-plugin-typescript. In other TS projects I never ran into issues like that. Maybe I'm holding it wrong, though.

unstubbable · 2018-01-23T21:52:34Z

@alan-agius4 Confirmed, so that won't work with rollup-plugin-typescript. But theses type of issues would now be covered by the test I added.

alan-agius4 · 2018-01-23T22:04:38Z

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> .

alan-agius4 · 2018-01-27T15:17:35Z

cc: @lukastaegert

lukastaegert

Hi, sorry for letting this rest here a few days. Thanks for taking care here, the test works nicely 👍
Once we updated the scripts (see my comment) this is good to go!

lukastaegert · 2018-01-27T22:50:14Z

    "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",


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.

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?

Alright, appveyor just answered my question. Will fix...

- add test:typescript script to test script - remove dist directory before build

lukastaegert · 2018-01-29T06:28:46Z

👍

unstubbable added 3 commits January 23, 2018 10:24

add a typescript test for dist/typings

13304f3

fix import of EventEmitter (events has no default export)

b7335f1

remove unused dependency buffer-xor

4965439

unstubbable mentioned this pull request Jan 23, 2018

Integration in TypeScript project broke again #1902

Closed

alan-agius4 approved these changes Jan 23, 2018

View reviewed changes

lukastaegert approved these changes Jan 27, 2018

View reviewed changes

unstubbable added 2 commits January 28, 2018 20:19

adjust npm scripts

65a713d

- add test:typescript script to test script - remove dist directory before build

add cash-mv/cash-rm for windows support

70fee6c

lukastaegert added this to the 0.55.2 milestone Jan 29, 2018

lukastaegert merged commit 70fee6c into rollup:master Feb 1, 2018

unstubbable deleted the typescript-test branch February 1, 2018 08:32

ghost mentioned this pull request Feb 1, 2018

Update rollup in /bundle from 0.52.1 to 0.55.2 src-zone/material#293

Closed

Uh oh!

Uh oh!

Conversation

unstubbable commented Jan 23, 2018

Uh oh!

alan-agius4 left a comment

Choose a reason for hiding this comment

Uh oh!

alan-agius4 commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

unstubbable commented Jan 23, 2018

Uh oh!

unstubbable commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alan-agius4 commented Jan 23, 2018 via email

Uh oh!

unstubbable commented Jan 23, 2018

Uh oh!

unstubbable commented Jan 23, 2018

Uh oh!

alan-agius4 commented Jan 23, 2018 via email

Uh oh!

alan-agius4 commented Jan 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukastaegert left a comment

Choose a reason for hiding this comment

Uh oh!

lukastaegert Jan 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unstubbable Jan 28, 2018

Choose a reason for hiding this comment

Uh oh!

unstubbable Jan 28, 2018

Choose a reason for hiding this comment

Uh oh!

lukastaegert commented Jan 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alan-agius4 commented Jan 23, 2018 •

edited

Loading

unstubbable commented Jan 23, 2018 •

edited

Loading

alan-agius4 commented Jan 27, 2018 •

edited

Loading

lukastaegert Jan 27, 2018 •

edited

Loading