Skip to content

Make is.js it's own independent module #55

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

Closed
jwerre opened this issue Nov 10, 2021 · 13 comments · Fixed by #108
Closed

Make is.js it's own independent module #55

jwerre opened this issue Nov 10, 2021 · 13 comments · Fixed by #108

Comments

@jwerre
Copy link
Contributor

jwerre commented Nov 10, 2021

is.js really slows down the tests. What do you guys think about moving it to it's own repository and adding it as a dependency?

Screen Shot 2021-11-10 at 8 42 09 AM

@jankapunkt
Copy link
Member

Hey @jwerre this code was written by me when I added these tests. The thing is, that the RegEx that is about to be tested allows nearly the whole unicode set and I do lots of permutations in order to cover them. This is course takes lot of time.

From my pov we should not remove them but we could update the docs and describe why this test is so slow (and give some background info).

We can also add an instant remedy by describing how to use regex pattern to filter out mocha tests that are not of focus right now: https://mochajs.org/#-grep-regexp-g-regexp

What do you think?

@jwerre
Copy link
Contributor Author

jwerre commented Nov 11, 2021

@jankapunkt Yes I remember when you wrote these test. Nice work by the way—I agree with you that they were needed. I guess I'm not clear why you wouldn't want to add is.js as a dependance under the same organization. It's not really oauth specific, it's not likely to change, and it would shave 5.5 sec off the unit tests. It just doesn't make sense to run these tests with every deployment of node-oauth-server when they can be run independently whenever there are changes to is.js.

@jankapunkt
Copy link
Member

Ah now I get, you want to add them as internal dependency. We could do this.

@jankapunkt
Copy link
Member

@jwerre @HappyZombies I would start this as a new repo under this org if you both agree

@jwerre
Copy link
Contributor Author

jwerre commented Nov 21, 2021

I put is.js into it's own repo. @HappyZombies would you mind publishing this to NPM? Once that's done I'll update the code and create a PR. @jankapunkt feel free to clone and make any changes you see fit.

@jankapunkt
Copy link
Member

@jwerre @HappyZombies I would first make some additions, like adding the namespace, updating the README, link the RFC docs etc. I'll link the PR here

@jankapunkt
Copy link
Member

See node-oauth/formats#1

@Uzlopak
Copy link
Collaborator

Uzlopak commented Dec 11, 2021

I am sorry to be nagging on the wording, but I think the name is wrong. it is not a "is-unicode" package because some of the validators are not unicode but simple ascii. It is actually something like oauth2-formats (naming similar to something like ajv-formats).

@jankapunkt
Copy link
Member

@jwerre what do you think about renaming? It's at least not published, yet.

@jwerre
Copy link
Contributor Author

jwerre commented Dec 11, 2021

Yes! Good call @Uzlopak

@jankapunkt
Copy link
Member

I will rename and then publish, so we can finally integrate it :-)

@jankapunkt
Copy link
Member

Published 1.0.0: https://www.npmjs.com/package/@node-oauth/formats
Let's keep this open until we integrated it into the main package

@jwerre jwerre mentioned this issue Dec 13, 2021
@jankapunkt jankapunkt linked a pull request Dec 14, 2021 that will close this issue
jankapunkt added a commit that referenced this issue Dec 14, 2021
…ats #55

Merge pull request #108 from node-oauth/oauth-formats thanks to @jwerre
@jorenvandeweyer
Copy link
Member

Resolved by #108

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants