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

docs: Add express-session-sqlite to README.md #733

Closed
wants to merge 1 commit into from
Closed

docs: Add express-session-sqlite to README.md #733

wants to merge 1 commit into from

Conversation

theogravity
Copy link
Contributor

I built this session store and would like to include it into the readme.

@theogravity theogravity changed the title Add express-session-sqlite to README.md doc: Add express-session-sqlite to README.md Mar 21, 2020
@theogravity theogravity changed the title doc: Add express-session-sqlite to README.md docs: Add express-session-sqlite to README.md Mar 21, 2020
I built this session store and would like to include it into the readme.
@ghinks
Copy link
Contributor

ghinks commented Mar 21, 2020

Thank you for your PR. There has been a little discussion around including so many references to 3rd party software. However the list on the session page has proven useful. What I would suggest is that we leave the PR open and monitor the uptake of your new module before we merge the PR.

@dougwilson
Copy link
Contributor

There may be a bit of a chicken-and-egg problem in that ask, as likely not being included in this list would hamper the ability for it to have uptake...

@theogravity
Copy link
Contributor Author

I understand. Thank you for taking a look at the PR.

@jonchurch
Copy link
Member

@ghinks I'm of the opinion it's up to Repo Captains when it comes to inclusion on the projects they steer. Like you pointed out, these types of repos are a special case where they really do need 3rd party modules. In this particular case, I'm more worried about an outdated list with dead projects, thereby funneling users into a defunct project by nature of it being prominently featured.

My 2 cents, I looked over this project and although I haven't used it, it ticks my boxes for a well thought out module.

However, there aren't any other sqlite express-session stores on NPM atm. If people do look for a sqlite session store on npm, they will find this one for sure.

@ghinks
Copy link
Contributor

ghinks commented Mar 21, 2020

Yes, after thought and investigation I do believe having 3rd party lists can become a liability as many will link to modules that are hardly used. I am going to re-raise an issue at the package maintenance meeting next week to see if better tooling to simply list out dependents is possible with help from the registry folks.

@dougwilson
Copy link
Contributor

Right, and even though this module in your head ticked a lot of the boxes, I have always believed it to be unfair to reject based on criteria not written. So far I have left the criteria empty.

If we want to come up with some kind of criteria, we should open a new issue. The worst is having a convo like this directly in a PR as it will end up lost since the lifecycle of closing it is unrelated to the conversation is a general sense. I'm referring to just the conversation around including the module list as a general issue vs including this specific module.

Currently this PR meets the only criteria laid to be included, so would seem on the surface to add "surprise" criteria after the fact :) that is just my 2 cents at least.

@ghinks
Copy link
Contributor

ghinks commented Mar 21, 2020

Agreed, I’m going to come up with the vision plan for the next meeting and this will certainly be a part of it.

@dougwilson
Copy link
Contributor

Hi @theogravity we didn't say we didn't want your PR per se, and I went to get it merged just now, but the repo was deleted. If you want to open a new PR, that would be welcome.

@dougwilson dougwilson closed this Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants