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

Port format-location to Typescript #4

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

CamillaTeodoro
Copy link

@CamillaTeodoro CamillaTeodoro commented Jun 27, 2023

Background

Story: https://goodeggs.atlassian.net/browse/SLK-15

In this PR we are migrating this repo from JS to TS following the instruction here: https://goodeggs.atlassian.net/wiki/spaces/ENG/pages/161841475/Porting+optionally+from+Flow+to+TypeScript

Changes

  • Migrate from npm to yarn
  • Migrate eslint configuration to latest version using goodeggs-toolkit
  • Decaffeinate
  • Port to typescript

@CamillaTeodoro CamillaTeodoro force-pushed the SLK-15-Port-format-location-to-Typescript branch from 473251f to 4ff0f3d Compare June 28, 2023 21:46
@CamillaTeodoro CamillaTeodoro marked this pull request as ready for review July 7, 2023 20:01
Copy link

@guilherme-vp guilherme-vp left a comment

Choose a reason for hiding this comment

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

If it's a quick win, we could set Node.js 18 everywhere to avoid old versions of Node.js that may not be LTS in the future?

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.test.ts Outdated Show resolved Hide resolved
Comment on lines +18 to +23
"@babel/cli": "^7.22.5",
"@babel/core": "^7.22.5",
"@babel/polyfill": "^7.12.1",
"@babel/preset-env": "^7.22.5",
"@babel/preset-typescript": "^7.22.5",
"@babel/register": "^7.22.5",

Choose a reason for hiding this comment

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

Uff, we could use tsc to build this lib instead of depending on 6 libs only to run babel... something for the future!

lib/index.js Outdated

Choose a reason for hiding this comment

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

Why the lib code is being exported? 🤔 Is it for use in web browsers in minified versions?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure why, but this file has been exported since the repo was created.
I did a quick search in all Goodeggs repos and didn't find any imports from this folder.

.vscode/launch.json Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
Comment on lines +1 to +5
// Temporary entrypoint wrapper for various `yarn` scripts to configure @babel/register for TypeScript
// TODO(murkey) Remove this file and:
// - Use this wrapper: https://github.com/deepsweet/babel-register-ts
// - Get a new version of @babel/register with built-in support: https://github.com/babel/babel/pull/6027
// - Get a version of babel that allows extensions to be configured via .babelrc/package.json: https://github.com/babel/babel/issues/3741

Choose a reason for hiding this comment

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

Another reason to use only tsc? 🤷🏻

@CamillaTeodoro CamillaTeodoro requested review from guilherme-vp and removed request for guilherme-vp September 4, 2023 17:21
@CamillaTeodoro CamillaTeodoro force-pushed the SLK-15-Port-format-location-to-Typescript branch from be79ddc to 01fde4c Compare September 4, 2023 21:23
@CamillaTeodoro
Copy link
Author

@guilherme-vp I tried to update the repo to node 18, but the test is crashing in Travis.

Screen Shot 2023-09-05 at 4 36 18 PM

Looks like Travis is not configurated to run node 18. I found in https://travis-ci.community/t/node-lib-x86-64-linux-gnu-libm-so-6-version-glibc-2-27-not-found-required-by-node/13655 an explanation for the error.

I think we should switch to node 18 when we move to Buildkite.

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.

2 participants