[Discussion] Work on new major release
kriswep opened this issue · 19 comments
First things first: I used gatekeeper in a little side project and it worked great for me.
But, looking at the source, the project starts to feel a little aged. This might turn potential users off and to newer solutions like micro-github or micro-auth.
Although they look like great work as well, I like gatekeeper's concept more, as it does not put the access token in the users query url.
So I propose working on a new major release/overhaul of gatekeeper.
Changes I have in mind (in no particular order):
- Using new ES2017 Syntax and changing node engine condition to v8 (which will be lts end of october). -> see #41
- Upgrading express or switching to another server framework like micro. Used version of express has some security flaws.-> see #43
- Committing to semver, so people know when to expect upgrade work. Right now, gatekeeper seems to be stuck at 0.1.0, which means we could release the proposed overhaul as 1.0.0. -> see #42
- Writing tests and using some kind of cloud CI (-> travis?). -> see #40
- Add linting and/or prettier. -> see #46
- Splitting up that one server.js file in smaller chunks ? -> see #41
- Adding contributing guidelines and enforcing a CoC, see community suggestions from github.
- Improving the readme, especially in regard to the configuration steps you have to do in github.
- Maybe adding some place for discussion like gitter or slack.
- ...
What do you think about that? Happy to discuss this and other ideas.
I would also like to participate in this upgrade.
I think this is a good undertaking, and I'm especially interested in updating the syntax, updating the node engine, and writing unit tests/incorporating CI.
I don't have a whole lot of availability through the end of the year but will do what I can to see this through. Since Gatekeeper is a relatively small repo, I think this can still move forward.
@kriswep how would you prioritize the steps here, and are there specific chunks you would particularly like to work on?
@kriswep I would recommend leaving the Node engine version requirement unchanged. I changed it recently to ~6.11.1 because Node announced a major security vulnerability in all of its active releases and released a patch for each of its releases. However, unless there is a reason why gatekeeper would be incompatible with version 6.11.1 following the proposed refactoring, I don’t think it’s a good idea to force people to upgrade their version of Node to continue using the most recent release of Gatekeeper. I think Gatekeeper should be as permissive as possible with its dependencies (including Node itself), unless there is a compelling reason to do otherwise.
I think the rest of your suggestions are great ideas, though. I don’t have nearly as much free time as I would like, but I could probably help out a little and do some code reviews.
Maybe we could also add a linter to the build process to enforce certain standards for syntax after the refactoring is complete. For example, preventing the use of the var keyword in favor of let or const.
@compumike08 I added linting to the list above, great point.
Reason for updating node would be the native support for async/await. I think the difference between requiring node 6 and node 8 isn't that hard. People who are on node 6 probably can upgrade to 8 rather easily. Plus any users on heroku or the like would get the upgrade basically for free.
Otherwhise we would need a transpiling step...
@dereklieu I'd start with creating a dev branch where we would target PRs against. So master would stay stable and usable. Probably also useful would be labels for issues regarding this. Then we should open issues regarding the different topics, which needs discussion, like server framework, test runner, lint tool, etc.
This ticket could be used as an overall planning / progress thread.
We could also add a note to the current readme to this discussion, so more people know and can get involved.
Plus maybe add me as collaborator with write access to this project, so I could help with this stuff. 😄
@kriswep You make a good point about native async/await support. I guess requiring Node version 8 once it’s LTS is okay.
I wouldn’t call the branch dev. Keep the branch name something descriptive about what the work being done on it is.
I thought of dev as a kind of consolidating branch, while we are working on different new features.
The named feature branches is what you would have in your working fork, right?
So, what do you think about a target branch for the refactor? Would like to send a first PR for discussion.
@kriswep @dereklieu Actually, I think the question of what to name the branch might be getting a bit ahead of ourselves. A decision should probably be made as to what Git workflow the project should follow. A good guide to several common Git workflows is available here.
Personally, I would recommend the Forking Workflow for this project. This workflow has the advantage of not requiring the owner of Gatekeeper to give additional people write access to the repository. I would make one small change to the workflow from what is described in the link above, though. I recommend that instead of PR from forked repos being made against the original Gatekeeper repo’s master branch as the workflow description states, we have a second branch off of master for the refactoring job which PRs can be made against. That way the current Gatekeeper code can remain in place on master and any emergency hotfixes can be made on the current version until the refactored version is ready to release. We could treat the second branch off of master as a release candidate branch.
What do you guys think?
I agree with the forking workflow and I'm ok calling the working branch either dev or development.
The practice where I work is similar, and we keep a forever-running development branch which is essentially our working branch to create pull requests against. Merging development into master becomes a new release with corresponding version bump.
I'd also advocate to open specific refactor tasks as individual tickets so they can be more easily assigned and tagged to pull requests. @kriswep since you created the initial list, I'd love if you could also create the tickets. Other users can then comment on individual tickets and we can decide which ones to work on (some are no-brainers, some might take some discussion).
To facilitate this I'm creating a development branch now.
As a last note, as Gatekeeper currently doesn't include a test suite and some of the changes we're proposing are quite invasive, I would advocate we develop a basic test suite first (or close to first) so that future work can include regression tests. I'm open to suggestions on frameworks, but prefer lightweight runners like https://github.com/substack/tape.
Sounds good to me regarding the workflow.
I’ll open individual issues tomorrow and reference them in the opening post.
Hello,
What's the state of this refactor ?
I'm a node.js developer and I plan to use gatekeeper, so contributing to it's improvement before using it seems like a legitimate usage of my time.
I think the correct way of working is creating a fork and using small feature-focused branches. That way changes can be increasingly incorporated into gatekeeper and several people can work on different topics at the same time.
I can easily advance the following tasks. Note that instead of listing the tasks, I'll list the branches I would use:
- feature/linting once we agree on the linting rules
- feature/continuousIntegration this task could include adhering to semver. I could use several tools like husky and commitizen for commit style enforcing and semantic-release for changelog and automatic version bumpint
- feature/codeRefactor here we could focus on splitting the server on smaller, testeable and re-usable units. Also I would love to move the code from callbacks to promises. I don't like async await at all, and using plain promises would allow us to stay at node 6
- feature/updateServer here we can update to latest express version or use a different one. Personally I like happijs, but express is fine
Let me think what do you think about the proposed workflow.
Regards
Anyone want to say something about my proposal ?
Thanks for chiming in. Sounds good to me, I guess every help would be appreciated.
AFAIK @dereklieu could create new branches on GitHub.
Guess you could start creating PRs to the dev branch.
I‘d say, work on CI features and/or code refactoring would be most helpful
Hey @danielo515 sorry for my late response. I think the proposed tasks are great. I have some preferences on linting but none particularly strongly held. If you have a lint rule-set suggestion, please put it forward in a ticket and we can go from there.
And yes, please do fork and open small feature branches, and pr against the development branch.
How is this going, all? Has development stopped? I stumbled upon this repo right before deciding to simply write my own implementation, but if it could be streamlined, I could possibly help with that some.
@muziejus we implemented a few of the proposed items but are still in need of a linter and tests. If you are open to contributing those, or any of the other tasks outlined here, I'd be happy to work with you on them.
OK. I'll get back to this in a bit, then. Thanks!
I made a serverless solution with Netlify Functions if that helps anyone: https://github.com/cadbox1/github-oktokit-oauth-netlify