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 #216

Merged
merged 7 commits into from
Sep 21, 2018
Merged

Add typescript #216

merged 7 commits into from
Sep 21, 2018

Conversation

miiila
Copy link
Contributor

@miiila miiila commented Sep 17, 2018

Introducing TypeScript will happen in 3 2 phases (flow verified in registry):

This PR introduces typescript support for source files and tests. One source file and test are converted in order to verify everything works as intended.

As a side effect, package-lock.json was added and npm prune was removed from Travis setup.

* Sourcode and tests had to be merged together, because conversion of one file led to different requires in tests

@adrukh
Copy link
Contributor

adrukh commented Sep 20, 2018

Let's not make this all chores? I understand that there are no functional changes, but this will not create a new release. Meaning that this will be pushed to our users together with another functional change, sometime later... Prefer to make sure it works as expected sooner, and without any additional changes. What do you folks think?

.snyk Show resolved Hide resolved
@michael-go
Copy link
Contributor

think now we should update .nvmrc to 8?

"eslint": "^4.11.0",
"nock": "^9.2.5",
"proxyquire": "^1.7.4",
"restify": "^4.1.1",
"sinon": "^1.17.2",
"strip-ansi": "^4.0.0",
"tap": "^12.0.0",
"tap-only": "0.0.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe if you remove tap-only, can also remove the check-tests scripts above? as it's purpose it to check for only() accidently commited

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michael-go tap supports .only() as well (https://www.node-tap.org/only/), that's why I have left it.

"tap-only": "0.0.5",
"tape": "^4.0.0"
"tap": "github:snyk/node-tap#alternative-runtimes",
"tape": "^4.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

is tape really needed? maybe it can easily be replaced with tap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH, I don't know, but don't want to do this decision now :-)

@miiila
Copy link
Contributor Author

miiila commented Sep 20, 2018

@adrukh In my opinion - it's not a functional change and we should be confident we haven't broken anything.

I also believe that adoption of CLI tool is not as fast as it happens in SaaS enviroment, so we would get a feedback after a while. Therefore, I don't believe feedback in a way no issue reported would necessarily means it's OK.

Additionally, if we would publish a new version ( with functional change) in the meantime and there would be a problem - what's a safe version for a rollback?

src/cli/index.ts Outdated
if (!runtime.isSupported(process.versions.node)) {
console.error(process.versions.node +
' is an unsupported nodejs runtime! Supported runtime range is \'' +
runtime.supportedRange + '\'');
console.error('Please upgrade your nodejs runtime version ' +
'and try again.');
process.exit(1);
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

with real ES6 modules (which is not the case here, but might be in the future with node 10), all the imports happen before the the script starts, so I think it's better to put all imports on top before any code to be more clear about it.

@miiila miiila merged commit 0f96166 into master Sep 21, 2018
@miiila miiila deleted the chore/typescript-cli branch September 21, 2018 09:37
@snyksec
Copy link

snyksec commented Sep 21, 2018

🎉 This PR is included in version 1.97.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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