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

Limit linkHeader length, throw error if exceeds #25

Merged
merged 2 commits into from
Dec 16, 2021

Conversation

assaf-benjosef
Copy link
Contributor

This limits the length of the linkHeader, and throws an error if the length exceeds a threshold (currently 2000, can change if needed).

Copy link
Owner

@thlorenz thlorenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please introduce those env vars and consider them in the code.
Additionally please add tests for this behavior as well as document the changes along with the env vars allowing to configure this.

Aside from that LGTM

index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@assaf-benjosef
Copy link
Contributor Author

Hi @thlorenz !
I added the env vars, tests and updated docs. And also moved this logic to a new func checkHeader to keep the exported function nice and simple.
lmk if there's anything else!

Copy link
Owner

@thlorenz thlorenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the nits/changes I found/requested.

README.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@assaf-benjosef
Copy link
Contributor Author

Please address the nits/changes I found/requested.

Done :)

@assaf-benjosef
Copy link
Contributor Author

Hi @thlorenz , did you get a chance to look at the changes?
Thanks 😄

@thlorenz
Copy link
Owner

thlorenz commented Nov 2, 2021

Currently swamped (new job) .. give me a week please.

@assaf-benjosef
Copy link
Contributor Author

Hi @thlorenz , did you get a chance to look at the changes? let me know if I can help in any way! (also hope you're enjoying the new job!)

@thlorenz
Copy link
Owner

Hey thanks for all the work and patience .. I've been super swamped with things lately but am still aware of this request.
I'll get to it ASAP.

@thlorenz thlorenz merged commit 72f05c7 into thlorenz:master Dec 16, 2021
@thlorenz
Copy link
Owner

Published as 2.0 (new major since this is a major behavior change). Thanks!

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 this pull request may close these issues.

2 participants