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

[README] Add nodejs as a system requirement #5017

Merged
merged 3 commits into from
Aug 13, 2019
Merged

[README] Add nodejs as a system requirement #5017

merged 3 commits into from
Aug 13, 2019

Conversation

johnsaigle
Copy link
Contributor

Brief summary of changes

As of (at least) version 21, it's necessary to use new-ish versions of NodeJS in order to properly package and run the LORIS front-end React components.

In lieu of a package or container or bootstrapping post-install script, we should at least make it clear that this is as essential to getting a non-development LORIS started as MySQL, Apache, etc.

@johnsaigle johnsaigle added Cleanup PR or issue introducing/requiring at least one clean-up operation Category: Documentation PR or issue that aims to improve the documentation (test plans, wiki, comments...) [branch] minor labels Aug 12, 2019
@@ -27,6 +27,7 @@ Deploy and log in with username *admin* and the password that's set up during de
* MySQL >= 5.7 (or MariaDB >= 10.3)
* PHP <b>7.2</b> or higher
* [Composer](https://getcomposer.org/)
* NodeJS <b>8.0</b> or higher
Copy link
Collaborator

Choose a reason for hiding this comment

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

How did you come to the conclusion 8.0 was the minimum version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Truthfully I haven't done extensive testing.

v10 is the latest LTS though and v12 is current so I think v8 is an appropriate ask.

Copy link
Contributor

@christinerogers christinerogers left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Question, just to check:
The following line: are we still asking people to run composer --no-dev if make does it for them?

@christinerogers christinerogers dismissed their stale review August 12, 2019 19:43

because i spotted something.

@christinerogers christinerogers added the Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects label Aug 12, 2019
@johnsaigle
Copy link
Contributor Author

@christinerogers No I don't think running composer directly is necessary anymore.

Copy link
Contributor

@christinerogers christinerogers left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, John.
The change looks good to me (I cannot vet the version number however), and at the same time let's update:

  • Remove line about running composer with --no-dev, if no longer relevant
  • Version 20 -> to 21 (rebase?)
  • The extreme bolding that is applied to Line 3 on the first paragraph, added by [Readme] Reformatting, add "Contributing" and "Help" sections #4082 for 20.3 -- This seems to be a by-product of adding the build status. if not intentional, could you find a way to remove this formatting: prob just move --- above the text

@christinerogers
Copy link
Contributor

I'll add a followup issue for the CentOS readme. If it should be tagged bugfix or critical to release please do.

@johnsaigle
Copy link
Contributor Author

@christinerogers I made the changes you asked for. Please feel free to edit the doc directly for small changes like this. It probably makes more sense to do that than to leave a note asking me to do it.

Testing if I can make a change on this Readme PR branch by @johnsaigle
@driusan driusan merged commit 54bb89d into aces:minor Aug 13, 2019
@ridz1208 ridz1208 added this to the 21.1.0 milestone Sep 6, 2019
@johnsaigle johnsaigle deleted the 190812-MentionNodeJS branch September 24, 2019 17:14
@ridz1208 ridz1208 modified the milestones: 21.1.0, 22.0.0 Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Documentation PR or issue that aims to improve the documentation (test plans, wiki, comments...) Cleanup PR or issue introducing/requiring at least one clean-up operation Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants