Skip to content

Test that the TypeScript files in our tests actually compile with tsc #56

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Dec 4, 2017
Merged

Test that the TypeScript files in our tests actually compile with tsc #56

merged 8 commits into from
Dec 4, 2017
+40 −21

Conversation

kyo-ago
Copy link
Contributor

@kyo-ago kyo-ago commented Dec 3, 2017 •
edited
Loading

Add tests for samples/*.d.ts

  • Rename samples/*.ts to samples/*.d.ts
    tsc does not recognize .ts as a type definition file.

kyo-ago
kyo-ago commented Dec 3, 2017
View reviewed changes
samples/overrides.d.ts
@@ -10,7 +10,7 @@ declare module overrides {
toString(): string;
}

export class B extends BLike {
Copy link
Contributor Author

@kyo-ago kyo-ago Dec 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

$ $(npm bin)/tsc samples/*.ts
samples/overrides.d.ts(13,26): error TS2689: Cannot extend an interface 'BLike'. Did you mean 'implements'?

kyo-ago
kyo-ago commented Dec 3, 2017
View reviewed changes
samples/import.ts
declare const hello: String;

declare module mod {
import { NestedImport } from "module.js";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

$ $(npm bin)/tsc samples/*.ts
samples/import.d.ts(14,34): error TS1147: Import declarations in a namespace cannot reference a module.

@kyo-ago kyo-ago changed the title add typescript test Add typescript test Dec 3, 2017
sjrd
sjrd requested changes Dec 3, 2017
View reviewed changes
.gitignore Outdated
@@ -5,3 +5,5 @@ target/
.classpath
.project
.settings/
node_modules
package-lock.json
Copy link
Owner

Choose a reason for hiding this comment

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

Add the appropriate slashes so that this does not pick other stuff by mistake:

/node_modules/
/package-lock.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in d6e7ff0

.travis.yml Outdated
script: sbt ++$TRAVIS_SCALA_VERSION samples/compile test
script:
- sbt ++$TRAVIS_SCALA_VERSION samples/compile test
- $(npm bin)/tsc samples/*.ts
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be *.d.ts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b4dcd54

.travis.yml Outdated
- . $HOME/.nvm/nvm.sh
- nvm install stable
- nvm use stable
- npm install typescript
Copy link
Owner

Choose a reason for hiding this comment

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

Please use fixed versions of Node.js and TypeScript. Something like

nvm use 8
npm install [email protected]

Otherwise the CI might start failing simply because a new version of Node.js or TypeScript is released.

.travis.yml
cache:
directories:
- $HOME/.ivy2/cache
- $HOME/.sbt/boot
- node_modules
Copy link
Owner

Choose a reason for hiding this comment

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

Is this standard practice? Is not dangerous? As in, if a new version of the CI script does not install some package anymore, it would still be in the cached node_modules/, and would therefore still be available, by accident?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is the standard practice.
Caching Dependencies and Directories - Travis CI
The Travis CI Blog: Speed Up Your Builds: Cache Your Dependencies
But I can understand your concerns as well. Do you want to delete it?

Copy link
Owner

Choose a reason for hiding this comment

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

OK let's keep it that way, then.

sjrd
sjrd requested changes Dec 3, 2017
View reviewed changes
Copy link
Owner

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

OK.

Could you just rebase your branch on top of the latest master, and make sure that there are no leftovers .ts in samples/? Since I have merged a few PRs since that PR was created, there are new .ts files in there (e.g., keyof.ts).

.travis.yml
cache:
directories:
- $HOME/.ivy2/cache
- $HOME/.sbt/boot
- node_modules
Copy link
Owner

Choose a reason for hiding this comment

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

OK let's keep it that way, then.

@kyo-ago
Copy link
Contributor Author

kyo-ago commented Dec 4, 2017

Rebase from master.
Imported #58.

sjrd
sjrd approved these changes Dec 4, 2017
View reviewed changes
Copy link
Owner

@sjrd sjrd left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks :)

@sjrd sjrd changed the title Add typescript test Test that the TypeScript files in our tests actually compile with tsc Dec 4, 2017
@sjrd sjrd merged commit 16a1e1c into sjrd:master Dec 4, 2017
This was referenced Dec 4, 2017
Merged
Merged
@kyo-ago kyo-ago deleted the test-for-typescript branch December 5, 2017 06:16
exoego referenced this pull request in exoego/scala-js-ts-importer Mar 23, 2018
@kyo-ago
Test that the TypeScript files in our tests actually compile with tsc (…
64ee37f 
…#56)
@exoego exoego mentioned this pull request Jun 16, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sjrd sjrd sjrd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kyo-ago @sjrd