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

Move to TypeScript #745

Merged
merged 68 commits into from
Jan 30, 2019
Merged

Move to TypeScript #745

merged 68 commits into from
Jan 30, 2019

Conversation

CvX
Copy link
Collaborator

@CvX CvX commented Jan 24, 2019

This PR converts the codebase to TypeScript.

Remaining tasks:

  • Resolve all “TODO: [TS]” comments
  • Add TS type definitions to electron-util, electron-debug, electron-dl, electron-context-menu, facebook-locales
  • Fix "build/files" in package.json, try out the build
  • Look into TS linting via ESLint/xo
  • Enable stricter TS rules in tsconfig.json, fix all errors

Fixes #740

@CvX
Copy link
Collaborator Author

CvX commented Jan 25, 2019

Any reason why source specifically? Isn't src a convention for TS projects?

That's what I settled on and use in other projects too. src goes back to the old Unix times where file paths were limited. source looks better and is more readable. No good reason to shorten it.

Should we change dist too? 😉

Jokes aside, what about dist-ts? I've named it that for a lack of a better idea at the time.
Should we move that inside dist, e.g. dist/js or is it better to keep those two separate? If so, at least I'd replace "ts" in that name to "js" (since that's what's inside).

@sindresorhus
Copy link
Owner

Yeah, I usually would go for distribution, but since we already have dist, which we cannot change as it's controlled by electron-builder, I think we should go with dist-js.

tsconfig.json Outdated Show resolved Hide resolved
source/emoji.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

@CvX Is there anything left to do? If not, I'll do a final review and merge.

@CvX
Copy link
Collaborator Author

CvX commented Jan 30, 2019

  1. Have to merge the master to this branch to fix the conflicts. I'll do it shortly.
  2. Ideally we would get facebook-locales definition out of the repo. I've submitted the PR (Add TypeScript definition wix-incubator/facebook-locales#6) but maybe it will go to DefinitelyTyped instead. In any case, we could update that later, after this gets merged into the master.
  3. I'm not sure about other .d.ts files (those that are Caprine-specific). They are just thrown out there, I feel like they could probably should be colocated with they code that uses these definitions, but that would have to wait for a bit of a refactoring (where we would group code of a single feature together, like we do with emoji now). Again, not a blocker IMO.

# Conflicts:
#	package-lock.json
@sindresorhus sindresorhus changed the title [WIP] Move to TypeScript Move to TypeScript Jan 30, 2019
@sindresorhus sindresorhus merged commit 3ec8c6b into sindresorhus:master Jan 30, 2019
@sindresorhus
Copy link
Owner

Wooooot!! Super nice work on this. It makes the codebase so much easier to work with. ❤️

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.

Move to TypeScript?
2 participants