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

refactor: add missing mts ext. support to checkly config #1019

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kiroushi
Copy link
Member

@kiroushi kiroushi commented Feb 17, 2025

I hereby confirm that I followed the code guidelines found at engineering guidelines

Affected Components

  • CLI
  • Create CLI
  • Test
  • Docs
  • Examples
  • Other

Notes for the Reviewer

Currently the CLI is able to read both JS/TS files, or .mjs when the app is declared as commonjs and needs to be interpreted as a ESM; but is missing the ability to do so when the config is written in TypeScript.

This PR adds the .mts extension to the list of candidates to read the config from to allow certain settings like https://www.typescriptlang.org/tsconfig/#verbatimModuleSyntax to work while in commonjs apps.

@kiroushi kiroushi requested a review from umutuzgur February 17, 2025 08:29
@sorccu
Copy link
Contributor

sorccu commented Feb 19, 2025

Are you sure that the current changes are enough? I don't think this works without changing loadFile too:

enum Extension {
JS = '.js',
MJS = '.mjs',
TS = '.ts',
}
export function loadFile (file: string) {
if (!existsSync(file)) {
return Promise.resolve(null)
}
switch (path.extname(file)) {
case Extension.JS:
return loadJsFile(file)
case Extension.MJS:
return loadJsFile(file)
case Extension.TS:
return loadTsFile(file)
default:
throw new Error(`Unsupported file extension ${file} for the config file`)
}
}

Without adding mts there, you'll just get an error saying checkly.config.mts is unsupported which makes no sense.

Additionally loadTsFile may potentially need some changes for modules (perhaps similar to loadJsFile), not completely sure about that without trying it out.

Either way I wouldn't really call this a refactor. I would like you to change this to feat instead because that's what it is, and it should therefore come with at least 1 test to prove that it works, or at minimum you must provide some proof that it worked for you locally.

Copy link
Contributor

@sorccu sorccu left a comment

Choose a reason for hiding this comment

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

See my comment

@sorccu
Copy link
Contributor

sorccu commented Feb 19, 2025

Also I would like to know the motivation for this change. I agree that it's good but just wondering what made you think of doing it.

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