Comments (33)
I have a clear opinion on this: master
is always reflecting the latest stable release
, so after a release being published with no bigger issues we should merge it to master. All development effort is going into development and release candidates are merged into release
. In such a case master
remains the base branch, because it reflects the latest production-stable version, no matter what mess has been created on other branches and this is why I think it should be push-protected.
from node-oauth2-server.
@jwerre good thing is that current tests already use mocha and chai so there is not much to change on their side. I already added NYC for coverage in 10 min or so
from node-oauth2-server.
@jankapunkt I was looking at the workflow you set up and I see that it's only testing on node 12. I wonder if it would be better to test in multiple versions? What do you think? Also, see previous comment.
from node-oauth2-server.
I agree with a lot of these, but let me go down each bullet point and explain what I think.
-
Tests and coverage (mocha, chai, sinon, istanbul)
- The tests here currently use mocha and sinon, and they are all currently passing. More tests of course wouldn't hurt and even code coverage would be nice. (Also FYI, istanbul has been deprecated)
-
Linter
- Currently the package uses js-hint, which I will be honest I never used before lol. I am personally more familiar with eslint, but an interesting thing is that the dev branch uses eslint , but the version 5 typescript branch uses eslint. Is one better with TypeScript than the other? I think we should prepare for TypeScript when it comes for the linter, so which ever is better for that let's go with it.
-
API Docs
- JSDoc 100%
-
Wiki, if desired
- Sure but not right now, we got other stuff to do.
-
README and badges
- Important too but let's do other code quality stuff first -- this can be last thing we do.
-
GitHub Actions for CI and CD
- 100%! But again, being honest, I never used them before -- so a bit learning curve for me but don't worry I will figure it out.
-
static analysis
- 100% but again, should be something we do last after we get "everything else" done.
-
should master / main be push-protected?
- Yes. I think all features/bug fixes should go into a
dev
branch, we then make PRs that go into dev. Then when dev is ready for a version, we create a release branch, then a PR to merge to master with the new release. After that we tag and publish. What do you think?
- Yes. I think all features/bug fixes should go into a
Action items with priority:
Below is what I think should be done in order of importance.
- HIGH Mergining strategy: My idea is master is push protected, all bugs and features go to dev. When dev is ready we make a PR or a release branch, review it. Then the PR goes to master with the new version. Tag and npm publish. Or for ease we can just use master as 'dev' and cut release as we go.
- HIGH GitHub Actions for CI and CD: Simplest right now would be for unit tests to pass at least for now. Then we can add more stuff later on.
- HIGH Linter: I think we should go with Prettier.
- MEDIUM Static Analysis: Important but we should do the above first.
- MEDIUM There are currently unit tests and those are passing. We can update the packages and add more after we do the above.
- LOW API Docs
- LOW Readme and badges
- LOW Wiki
from node-oauth2-server.
Mergining strategy
I think the branching is good, we should always release into release
so we can have CI run extra integration tests with all adapters before publishing. Dev can run only standard tests.
GitHub Action
I have experience with GH Actions and can provide a PR.
Linter
I have no experience with linter and typescript so maybe someone else who is more experienced in this combo starts a PR?
Static Analysis
Yes I'm good with that
API Docs
I can provide a PR later
Readme and badges and wiki
let's focus on that later
from node-oauth2-server.
Sounds good, I will clean up the dev branch and add protection to master so no one can push directly.
Sadly we lose whatever was on dev but worse case we just look at the old dev branch from the original repo.
from node-oauth2-server.
@jankapunkt should development be the default branch? I feel like doing so would kinda make master redundant...which then begs the question of why even have a development branch 🤷
from node-oauth2-server.
@HappyZombies I added a PR regarding branching and contribution guidelines and I would ask to you to work on the maintainers part a bit (which is more closely related to the branching strategy) and let me know what you think of the current state.
from node-oauth2-server.
I'm also a fan of:
- mocha, chai or should
- eslint
- jsdocs
from node-oauth2-server.
I just realized the should.js
is the main the assertion library but the repo has been archived! I wonder if continuing to use it would be the best way to future-proof. What do you guys think? Do you think we could just use Chai's should interface as a simple swap in replacement. e.g.:
// var should = require('should'); // old way
const should = require('chai').should()
What a lot of people don't like about should is that it extends the Object.prototype
. I've used should
for years and never had any problem but I get the complaint. That said it could be a lot of work change all the tests.
from node-oauth2-server.
I personally love the Chai asserter and I think together with Mocha it has the widest spread of expertise so many people should be familiar with it (yes very meta).
I think we should focus on this before we do the other issues because this is a fundamental thing for our CI and future contributions
from node-oauth2-server.
What about using expect if we need to rewrite them anyway?
from node-oauth2-server.
I don't think you'd need to rewrite anything if you just use Chai's should interface. I'll check it out now and see what happens.
from node-oauth2-server.
I just did a find for should = require('should');
and replace with should = require('chai').should()
in the entire test/unit
directory and all but one test passed.
from node-oauth2-server.
Another code quality tool we could leverage to prevent secrets being pushed: https://github.com/trufflesecurity/truffleHog
For tests we could set some fuzzing tools in perspective, that run before a new release may be published: https://github.com/google/oss-fuzz
from node-oauth2-server.
Are we going to move forward with ESLint? Perhaps we should start a new issue for this topic and discuss configuration?
from node-oauth2-server.
I think we should go for ESLint, I thought about prettier, but having it find code errors will help us catch those undefined errors quickly.
In the original project, they started using ESLint but, never got merged to master. Perhaps this is something we can benefit from?
https://github.com/oauthjs/node-oauth2-server/blob/dev/.eslintrc
from node-oauth2-server.
I agree let's create a separate issue and discuss a potential eslint config
from node-oauth2-server.
I think we should set Pull Requests to require at least one passing review to be merged, for critical things like new features or feature changes even two
from node-oauth2-server.
@jankapunkt I agree, I will make this change..if i can? lol
from node-oauth2-server.
@HappyZombies there should be a setting somewhere that pull requests need approval
from node-oauth2-server.
@HappyZombies I think it's part of branch protection rules https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule
from node-oauth2-server.
Ah I see, I need to make the rule for development branch specifically, got it thanks!
from node-oauth2-server.
@jankapunkt Done. btw do you not have access to those settings? I can up your privileges if needed.
from node-oauth2-server.
What about actually merge
or rebase
for merge strategy? Each has its own pros and cons. The merge produce too obfuscated network of branches in history while rebase gives a linear chain of commits.
from node-oauth2-server.
@oklas correct but I also messed up a complete project with rebasing so I stick mostly with merge or squash merge. In which situation I would choose a rebase over merge when approving a pull request?
from node-oauth2-server.
I'm okay with merging and squash merging. Rebasing...eh, but personal preference (plus I think more people know about merging, but this shouldn't be a factor in us deciding this)
from node-oauth2-server.
I moved details and discussion about situations with rebase
and merge
let's refer #31. ESLint also moved to its ticket. The next high question is CI/CD.
from node-oauth2-server.
Is it safe to delete the travis.yml
?
from node-oauth2-server.
@jwerre let's take the Node version convo to #34
from node-oauth2-server.
Once #18 is merged in, any PR moving forward (not the current ones) will have to abide by the commit and PR conventions.
from node-oauth2-server.
Also i want to mention that the integration tests are imho basically pointless.
Currently everything is mocked with pointless stubs. How you want to check if the scope is valid, if the method always returns a specified return value.
It would be better to implement an in memory model. And write unit tests which check the correct behavior of the memory model and then run tests against it. Thus showing us some real issues.
from node-oauth2-server.
Closing this issue as other individual items have been made and I believe we have addressed all we could here.
from node-oauth2-server.
Related Issues (20)
- An option to require PKCE parameters HOT 6
- Does this library support user approval dialog during authorization code grant? HOT 28
- State of this project? HOT 21
- Is implementation of `verifyScope` required? HOT 17
- generateAuthorizationCode not being awaited HOT 3
- TypeScript: Remove callback from types in 5.x HOT 4
- Move all ES5 style classes into ES6+ style class HOT 2
- getClient called with non-decoded secret/client HOT 3
- [Documentation] revokeAuthorizationCode argument should be named `code.authorizationCode`, not `code.code` HOT 4
- Client Credentials broken in 5.0.0-rc.1 HOT 12
- Insufficient integration tests HOT 3
- Contribution guidelines do not cover how to PR fixes for docs HOT 2
- wrong typing for revokeToken argument HOT 26
- PR #197 fix removed after merge HOT 3
- Typings for `validateScope` don't correctly reflect that `scope` arg can be undefined
- `authenticate` endpoint still expects `scope` as a `string` instead of `string[]` HOT 4
- PKCE Refresh token HOT 12
- Poor example code for verifyScope in docs
- `AuthenticateOptions.scope` type is declared incorrectly
- v5.1`AuthorizationOptions.scope`Breaking Change HOT 3
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from node-oauth2-server.