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

Update npm-publish.ts #13

Closed
wants to merge 2 commits into from
Closed

Conversation

ShadyNagy
Copy link

@ShadyNagy ShadyNagy commented Sep 7, 2020

Fix #11
This change to fix error if there are not any packages in the NPM as it is giving error on that function getLatestVersion and if the checkVersion flag set to false then no use to get version and compare.

This change to fix error if there is not any packages in the NPM as it is giving error on that function getLatestVersion and if the checkVersion flag set to false then no use to get version and compare.
@nikitaeverywhere
Copy link
Contributor

nikitaeverywhere commented Sep 9, 2020

Hello!

I made the same PR to fix #11 two day ago (#12). Why one more?

Moreover, your one seems to remove the version at all:

oldVersion: "",

@ShadyNagy
Copy link
Author

@ZitRos

Hello!
I did not see your PR when I made that PR and I do not think there is any problem if there is 2 PR doing the same thing in a different way as this will give the author to choose between.
There is no oldVersion if you publish 1st time.
Any way I will be happy even your PR publish but I need that problem to be fixed soon as I did not find till now a good complete package every package has a small problem!!!

@nikitaeverywhere
Copy link
Contributor

True :) Let's wait for the author to review.

@ShadyNagy
Copy link
Author

@JamesMessinger

We are waiting you as there are 2 PR (#13 , #12 ) to choose between them or you can fix that problem by yourself as the package not working with any first release in npmjs.

@varunsridharan varunsridharan mentioned this pull request Sep 28, 2020
@JamesMessinger
Copy link
Member

Thank you for the PR, @ShadyNagy! I ended up merging @ZitRos's PR instead, as it was more in-line with what I envisioned, but I'm grateful to both of you for your contributions. 🙏

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.

Initial publish
3 participants