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

CLI: Update CliClient Node dependency to 10+ #2177

Merged
merged 1 commit into from
Dec 18, 2019

Conversation

joeltaylor
Copy link
Contributor

@joeltaylor joeltaylor commented Dec 12, 2019

The CliClient does not build correctly with Node 8 anymore and seems to to be the root of installation failures for many reported issues:

In addition, Node 8 is reaching EOL 2019-12-31

Building and installing with Node 10 succeeds and should alleviate the issues.

@@ -1,6 +1,6 @@
{
"name": "joplin",
"version": "1.0.149",
"version": "1.0.150",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really a Node expert, is this version bump expected when upgrading the node version in package.json? If not, I can remove this change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, this (the version bump) is done during the release process. Please revert this change.

Copy link
Collaborator

@tessus tessus left a comment

Choose a reason for hiding this comment

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

You don't have to change the html files because they are generated from the .md files. But it doesn't matter either. Just a thing to remember for the future.

As mentioned earlier inline, please revert the change to the package-lock.json file. (Never change the file manually, nor commit automatic changes to this file - unless directed by Laurent.)

Other than that... LGTM.

@tessus tessus added the cli CLI app specific issue label Dec 12, 2019
@joeltaylor
Copy link
Contributor Author

@tessus thanks for the feedback -- I'll remember both points in the future! I've reverted the package-lock.json and the html changes.

Copy link
Collaborator

@tessus tessus left a comment

Choose a reason for hiding this comment

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

Thank you.

Oops, missed something. I added an inline comment.

readme/terminal.md Outdated Show resolved Hide resolved
@joeltaylor joeltaylor force-pushed the update-cliclient-node branch 3 times, most recently from fd475be to ad2af0d Compare December 17, 2019 16:49
@tessus
Copy link
Collaborator

tessus commented Dec 17, 2019

@laurent22 This is ready to be merged. The 2 html files don't matter, in fact in this case we don't have to rebuild the web site right away...

@laurent22
Copy link
Owner

I'd prefer if the commits don't include unnecessary files though. I know it might be confusing that they are in the repo (they are required by GitHub Page) but that's why I've added a big warning on top of the files:

!!! WARNING !!!
This file was auto-generated from readme/terminal.md and any manual change
made to it will be overwritten. To make a change to this file please modify
the source Markdown file:
https://github.com/laurent22/joplin/blob/master/readme/terminal.md

@joeltaylor any chance you could remove the two HTML files please?

@joeltaylor
Copy link
Contributor Author

@laurent22 Most certainly, I originally removed them but must not have force pushed. Sorry about that!

@joeltaylor
Copy link
Contributor Author

HTML files removed, apologies for missing that big warning comment. 👍

@laurent22
Copy link
Owner

Perfect, thanks @joeltaylor!

@laurent22 laurent22 merged commit 172afb0 into laurent22:master Dec 18, 2019
@duarteocarmo
Copy link

Hey guys, installing with brew install joplin, brew installs node 10.16.3 but I still get
Screen Shot 2020-03-02 at 17 37 37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli CLI app specific issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants