search

phusion / passenger_library

Phusion Passenger documentation
https://www.phusionpassenger.com/docs
Other
48 stars 111 forks source link

Meteor 16 update: closes #64 #66

Closed serkandurusoy closed 6 years ago

serkandurusoy commented 6 years ago

@OnixGH In light of https://github.com/phusion/passenger_library/issues/64 I've prepared this PR.

Although, this is my first time working on anything ruby related, and the shared jquery hide/show magic applied across pages for meteor versions make it hard for me to properly debug whether or not my changes are fully proper.

I'd love it if you reviewed. I tried to make small individual commits to make it easier to drill down if needed.

cc @dr-dimitru

This PR does not contain anything in reference to https://github.com/phusion/passenger/issues/1996 because it looks like @OnixGH may be onto something in store for an out of box solution to that ;)

serkandurusoy commented 6 years ago

@dr-dimitru I've invited you to my fork as a collaborator. Please feel free to commit && push to that branch for your suggestions.

Thank you for reviewing! :)

OnixGH commented 6 years ago

@serkandurusoy thanks :) I've put it on my list, did you test the output with rake build?

edit: I mean, run the build and then browse to the pages you changed to manually check

serkandurusoy commented 6 years ago

@OnixGH yes, and it both builds without any warnings/errors and the resulting static site looks like it behaves correctly on those places I've made edits.

serkandurusoy commented 6 years ago

@OnixGH sorry I was missing for a whole week. Anyway I think I've covered your requests with these latest commits, let me know what you think.

cc @dr-dimitru

serkandurusoy commented 6 years ago

@OnixGH @dr-dimitru do you guys think we can push this forward and at least merge as is if current changeset is not too bad? or do you want to keep the conversation going?

dr-dimitru commented 6 years ago

@serkandurusoy +1 for merge and starting a new issue ticket for further discussion. @OnixGH WDYT?

OnixGH commented 6 years ago

@dr-dimitru makes a good point that I hadn't realized so far; hidden somewhere in the Meteor guide there is indeed the suggestion of a tight versionlock.

If you use a mis-matched version of Node when deploying your application, you will encounter errors!

In general, the purpose of the page is to help beginners follow a simple flow that ties the necessary knots together. For example, the Meteor guide doesn't really help with Node, and the Node doc doesn't help with Meteor, so we provide basic instructions so people don't get bounced around.

However, it's not a great prospect to complicate our tutorial for beginners with all these individual Meteor/Node matches. On top of that there's no nodesource scripts for certain combinations (like 5.5.1 / 8.8.1) so the page would need even bigger changes.

In any case, the edits by @serkandurusoy are good for the latest Meteor (can you squash them a little where appropriate?) so let's merge those.

For future edits I would suggest changing the page to only provide instructions for the latest Meteor version and tell people to either upgrade to that first, or use meteor node -v and go to the Node.js page to figure out how to install that particular Node (this page seems like a good start).

dr-dimitru commented 6 years ago

In general, the purpose of the page is to help beginners follow a simple flow that ties the necessary knots together. For example, the Meteor guide doesn't really help with Node, and the Node doc doesn't help with Meteor, so we provide basic instructions so people don't get bounced around.

👍👌

Note: Meteor just got updated, v1.6.0.1 - this release is very recommended as requires to upgrade node to v8.9.3, which covers important security breach. I'm still highly confident to ask users to check latest Meteor's requirements on Node.js and NPM as it might be changed at any time.

OnixGH commented 6 years ago

Yep, 8.9.3 is the latest LTS, and in this case the debsource links from @serkandurusoy's edits already automatically result in that version (i.e. no doc change needed).

This is also why I don't like the narrow versionlocking too much (you probably always want at least the latest patch release).

serkandurusoy commented 6 years ago

@OnixGH do you have an ETA about when this can be merged or do you think we need to iterate more on this?

OnixGH commented 6 years ago

@serkandurusoy I was waiting for your squash action (".. the edits by @serkandurusoy are good for the latest Meteor (can you squash them a little where appropriate?)"), sorry if that wasn't clear.

serkandurusoy commented 6 years ago

Hey @OnixGH sorry for missing that. There you go: https://github.com/phusion/passenger_library/pull/66/commits/0a60ca9d235ee78083051a9052b38058de5ed1b7

dr-dimitru commented 6 years ago

Good job everyone, thanks for the merge.