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

fix: running prism cli threw exception [SL-2021] #190

Merged
merged 1 commit into from
Mar 8, 2019

Conversation

chris-miaskowski
Copy link
Contributor

  • Class constructor Command cannot be invoked without 'new'

SL-2021

- Class constructor Command cannot be invoked without 'new'

SL-2021
@XVincentX
Copy link
Contributor

@chris-miaskowski

Apparently it does not seem to be solving the problem. I've cloned this branch and ran yarn build and then the command.

Also, given the bug I think it would be nice to at least run a test that the CLI runs — even simply printing the Prims version.

@chris-miaskowski
Copy link
Contributor Author

@chris-miaskowski

Apparently it does not seem to be solving the problem. I've cloned this branch and ran yarn build and then the command.

Also, given the bug I think it would be nice to at least run a test that the CLI runs — even simply printing the Prims version.

Hmm interesting, are you sure you didn't have outdated lib folder in the CLI package?
I have tested this locally and it works...

Copy link
Contributor

@XVincentX XVincentX left a comment

Choose a reason for hiding this comment

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

You're right! I cleaned the whole thing and it seems like it's working correctly now.

I'd still appreciate a minimal test on the CLI (simply printing the version or something) but it can be a next step.

@chris-miaskowski
Copy link
Contributor Author

You're right! I cleaned the whole thing and it seems like it's working correctly now.

I'd still appreciate a minimal test on the CLI (simply printing the version or something) but it can be a next step.

I tried implementing a simple unit test once and it was a pain... Jest had some problems with closing the child process as far as I recall. But yeah we should have tests for this :(

@chris-miaskowski chris-miaskowski merged commit 1893ccc into develop Mar 8, 2019
@chris-miaskowski chris-miaskowski deleted the bug/sl-2021/change-cli-build-target branch March 8, 2019 13:55
@XVincentX
Copy link
Contributor

Alternatively it could be just a command on the CI ran after the tests. That'd at least check that the CLI loads correctly.

@chris-miaskowski
Copy link
Contributor Author

Oh, that's actually a good idea :) and we could check the shell's return code

@XVincentX
Copy link
Contributor

XVincentX commented Mar 8, 2019 via email

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