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

Migrate from Flow to TypeScript #5

Merged
merged 5 commits into from
Oct 4, 2019
Merged

Migrate from Flow to TypeScript #5

merged 5 commits into from
Oct 4, 2019

Conversation

mohoff
Copy link
Contributor

@mohoff mohoff commented Sep 21, 2019

Goal of this initial PR is to have a drop-in replacement for our existing @brickblock/strong-config. This means primarily that all flow-types are converted to Typescript typings. Additionally, typescript-specific project setup, CI setup, unit tests, dynamic type generation for Config object.

Donezo:

  • Setup Typescript project with tsconfig.json and eslint+prettier.
  • Migrate code base to Typescript
  • Refactored code into more composed utils
  • Generate Typescript types instead of Flowtypes for config object
  • Added test. Coverage above 80%

Not in the scope of this MR:

  • Make everything super nice
  • Enduser-ready README, CONTRIBUTIONS, LICENSE
  • line-perfect job definitions in .gitlab-ci.yml

These and more should be extracted into separate GitHub issues and addressed in future, hopepfully smaller, PRs

@mohoff mohoff force-pushed the feat/migrate-to-ts branch 7 times, most recently from bcf2091 to eb9791f Compare September 24, 2019 13:18
@chapati23
Copy link
Collaborator

Great, super excited to start working on this out in the open!

Before we move this PR out of WIP, i have the following requests:

  • Add a little PR description on what this PR focusses on and therefore what reviewers should focus on. I assume a great chunk of the changed files are the result of bbk sync, so I'm not sure what I can ignore and where TypeScript-specific changes were introduced here
  • Maybe also make explicit what's NOT in scope of this PR. E.g. if CI setup will be done in a separate PR that's totally fine (didn't see any checks in GitHub's UI, hence my impression that nothing was running)
  • As this is our very first TypeScript MR of all times ever, it would be nice if you could add some quick thoughts on why you went with your chosen setup in case you deviated from any TypeScript defaults. E.g. is lib the standard build folder for TypeScript? If not, what made you chose it? Stuff like this.

.gitlab-ci.yml Outdated Show resolved Hide resolved
lint-staged.config.js Outdated Show resolved Hide resolved
scripts/healthcheck.ts Outdated Show resolved Hide resolved
src/load.ts Outdated Show resolved Hide resolved
src/load.ts Outdated Show resolved Hide resolved
src/load.ts Outdated Show resolved Hide resolved
src/utils/generate-type-from-schema.ts Show resolved Hide resolved
src/utils/generate-type-from-schema.ts Outdated Show resolved Hide resolved
src/utils/substitute-with-env.ts Show resolved Hide resolved
@chapati23 chapati23 changed the title Feat/migrate to ts Migrate from Flow to TypeScript Sep 26, 2019
Copy link
Collaborator

@chapati23 chapati23 left a comment

Choose a reason for hiding this comment

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

have to get used to this double-submission of review comments...

.eslintrc.js Show resolved Hide resolved
.gitignore Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/utils/hydrate-config.ts Show resolved Hide resolved
src/utils/generate-type-from-schema.ts Outdated Show resolved Hide resolved
src/utils/hydrate-config.ts Show resolved Hide resolved
src/utils/read-file.ts Outdated Show resolved Hide resolved
src/utils/read-file.ts Outdated Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
@mohoff mohoff force-pushed the feat/migrate-to-ts branch 5 times, most recently from 82b7f2b to 1c54a34 Compare September 27, 2019 10:13
@mohoff mohoff marked this pull request as ready for review September 27, 2019 10:14
@mohoff mohoff force-pushed the feat/migrate-to-ts branch 2 times, most recently from fde564b to a8cb07d Compare September 27, 2019 10:58
Copy link
Collaborator

@chapati23 chapati23 left a comment

Choose a reason for hiding this comment

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

still figuring out what useful comment to write here when submitting a review :D

.npmignore Outdated Show resolved Hide resolved
.gitlab-ci.yml Show resolved Hide resolved
src/utils/hydrate-config.ts Show resolved Hide resolved
src/utils/hydrate-config.ts Show resolved Hide resolved
src/utils/substitute-with-env.ts Show resolved Hide resolved
tsconfig.json Show resolved Hide resolved
@PaddyMc
Copy link

PaddyMc commented Oct 2, 2019

Should we add a CONTRIBUTING.md file? It may help attract open-source devs.

Reasoning
Example

Maybe not for this PR tho, wdyt?

README.md Show resolved Hide resolved
@chapati23
Copy link
Collaborator

Should we add a CONTRIBUTING.md file? It may help attract open-source devs.

Reasoning
Example

Maybe not for this PR tho, wdyt?

Great idea, can you create a new ticket for this @PaddyMc ?

@mohoff mohoff merged commit 692e24a into master Oct 4, 2019
@chapati23 chapati23 deleted the feat/migrate-to-ts branch October 4, 2019 11:42
@mohoff mohoff mentioned this pull request Oct 4, 2019
2 tasks
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.

4 participants