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

Add TypeScript support (old; don't merge) #49

Closed
wants to merge 7 commits into from

Conversation

andrewsantarin
Copy link
Contributor

I've got a working example of this using a test package using the TypeScript definitions in @andrewsantarin/staticify

The working example: https://github.com/andrewsantarin/staticify-test

Clone it, install dependencies, see the source on a TypeScript-capable IDE and run locally to see it in action.

In all honesty, it's overkill to actually prove that the types work on an actual Node.js project, but I have other uses for it.

@andrewsantarin
Copy link
Contributor Author

andrewsantarin commented Nov 29, 2019

Okay, it looks like TypeScript on Windows absolutely hates it when I use wildcards in the tsc command.

image

I didn't see this earlier because used macOS, which didn't throw any errors because the terminal supports wildcards, I think? (GitHub/typescript, Reddit, StackOverflow). Then I switched over to Windows and surely enough, found the problem.

I found out that there are two ways to solve this:

1. Use tsconfig.json and let this file handle the wildcards for that, at the cost of add additional file.

On package.json

{
  "scripts": {
    "tsc": "tsc"
  }
}

On test/index.ts

import staticify, { StaticifyOptions } from 'staticify';

Then add a tsconfig.json file (like so) so that any TypeScript files under the /test folder will be picked up

{
  "compilerOptions": {
    "module": "commonjs",
    "lib": [
      "es6"
    ],
    "strict": true,
    "noEmit": true,
    "baseUrl": ".",
    "paths": {
      "staticify": [ // Here be it
        "."
      ]
    }
  }
}

2. Explicitly mention which files to pass to tsc, at the cost of manually adding a new file each time we need it.

On package.json

{
  "scripts": {
    "tsc": "tsc --esModuleInterop --noEmit test/index.ts"
  }
}

On test/index.ts

import staticify, { StaticifyOptions } from '../index';

@rakeshpai
Copy link
Member

Thanks for this. I'll review over the weekend.

@andrewsantarin
Copy link
Contributor Author

All right, @rakeshpai.

Do let me know which of the two approaches you'd rather go with. I took the 2nd one; the typecheck is simple and doesn't really need tsconfig.json yet.

@rakeshpai
Copy link
Member

Hi @andrewsantarin I like the 2nd approach too. Should be good enough for the time being.

Just a thought - rather than creating a .d.ts file, wouldn't it be possible to use JSDoc comments and use tsc to generate the declaration files? Example: https://dev.to/open-wc/generating-typescript-definition-files-from-javascript-5bp2 (I've never used this technique myself.)

The benefit of this is that it's going to be easier for a JS developer to maintain, assuming no familiarity with TS.

@andrewsantarin
Copy link
Contributor Author

@rakeshpai

The article you mentioned concludes that TypeScript has not yet made this an official feature yet.

While, yeah, we could use JSDoc to generate to generate *.d.ts, we require a tool to do that for us:

  • a fork of typescript exists, but it has no guarantee that it'll be supported in the future (with the fellow engineers I work with, this is a red flag).
  • phaser uses its owntsgen tool, which looks like a lot of work and only pays off if the source code is huge. We only have here, like, what, one index.js?

Also, consider the fact that you have a dependency with modules such as send. I'm not sure how that works with JSDoc comments. Hopefully, you know how to typify options.sendOptions as SendOptions correctly on JSDoc, because I sure as hell don't. 😅

tl;dr: It's possible, but a resounding "impractical given the hands we have", in my opinion.

@XhmikosR
Copy link
Collaborator

XhmikosR commented Dec 3, 2019

@andrewsantarin you need to double quote the globs in npm scripts. Never forget this :P

@andrewsantarin
Copy link
Contributor Author

andrewsantarin commented Dec 4, 2019

@andrewsantarin you need to double quote the globs in npm scripts. Never forget this :P

Sorry, @rakeshpai, I don't understand what you mean. Like this?

"tsc": "tsc --esModuleInterop --noEmit \"test/index.ts\"

@XhmikosR
Copy link
Collaborator

XhmikosR commented Dec 4, 2019

#49 (comment)

The reason is the missing double quotes.

@rakeshpai
Copy link
Member

#49 (comment)

Ok, seems fair. This PR LGTM. @XhmikosR does this look fine to you?

@XhmikosR
Copy link
Collaborator

XhmikosR commented Dec 6, 2019

I don't use Typescript myself so I can't comment on the change itself.

@XhmikosR
Copy link
Collaborator

XhmikosR commented Dec 6, 2019

Also, I'm not sure about the added test. Why do we need it? I'd rather have less scripts personally.

@andrewsantarin
Copy link
Contributor Author

andrewsantarin commented Dec 16, 2019

@XhmikosR It's there so that if the machine that's running the tests doesn't have, let's say a TypeScript-compatible editor like VSCode, you can still verify the types work. Personally, I don't need it because VSCode is exactly what I use, so if simpler scripts is seriously the way to go, then ping me so that I can remove the tsc script.

Also, @rakeshpai @XhmikosR, please excuse my being late! Hope you're still keen on accepting this PR.

@andrewsantarin andrewsantarin changed the title Add TypeScript support Add TypeScript support (old) Jun 18, 2020
@andrewsantarin andrewsantarin changed the title Add TypeScript support (old) Add TypeScript support (old; don't merge) Jun 18, 2020
@XhmikosR XhmikosR closed this Jun 18, 2020
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.

3 participants