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

Port recent commits from http-parser #38

Closed
wants to merge 3 commits into from

Conversation

fanatid
Copy link
Contributor

@fanatid fanatid commented Jan 7, 2020

Issue #12

nodejs/http-parser@01da95f already in tests, but I moved them from invalid.md to content-length.md
nodejs/http-parser@3502589 [x] added
nodejs/http-parser@cd88eef [x] need improvements

Early feedback appreciated. Is this OK that mdgator require extra newline? For new test I was need \r\n\r\n, but with mdgator I was able receive it only with 2 empty lines (which give 3 x \r\n, right?).

Whole project (and deps) is just WOW, I do not understand how it's possible to create it haha. Spent few hours for adding simple 5 lines.

@indutny
Copy link
Member

indutny commented Jan 7, 2020

Whole project (and deps) is just WOW, I do not understand how it's possible to create it haha. Spent few hours for adding simple 5 lines.

I can't tell whether you are happy or sad about it from the wording 😂

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM with a nitpick. Great work!

src/llhttp/http.ts Outdated Show resolved Hide resolved
@fanatid
Copy link
Contributor Author

fanatid commented Jan 7, 2020

Not sure how port third commit, my way is super shitty and makes things more unreadable.
Main problem that span.headerValue.start / span.headerValue.end is always in chain, but we do not need it for header state in content-length on second line.

Maybe it worth land first 2 commits and move not good solution to another PR for future?

@indutny
Copy link
Member

indutny commented Jan 8, 2020

I concur!

@fanatid
Copy link
Contributor Author

fanatid commented Jan 8, 2020

Should I submit first two in another PR or you cherry-pick them?

indutny pushed a commit that referenced this pull request Jan 8, 2020
PR-URL: #38
Reviewed-By: Fedor Indutny <fedor@indutny.com>
indutny pushed a commit that referenced this pull request Jan 8, 2020
Ported from http-parser:
nodejs/http-parser@3502589

PR-URL: #38
Reviewed-By: Fedor Indutny <fedor@indutny.com>
indutny pushed a commit that referenced this pull request Jan 8, 2020
Ported from http-parser:
nodejs/http-parser@cd88eef

PR-URL: #38
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@indutny
Copy link
Member

indutny commented Jan 8, 2020

Landed in 741a93c, df5f3ab, and 34835d3.

@indutny indutny closed this Jan 8, 2020
@fanatid
Copy link
Contributor Author

fanatid commented Jan 8, 2020

Oh, so everything was go to master?
Did not thought about land 34835d3 but if this in master, I think I'll better add some comments with explanations, because things going more tangled :(

@indutny
Copy link
Member

indutny commented Jan 8, 2020

Ooops. This wasn't my intention. Reverting! Thanks.

@indutny
Copy link
Member

indutny commented Jan 8, 2020

Partially reverted in d167b03.

@fanatid
Copy link
Contributor Author

fanatid commented Jan 8, 2020

Thanks!

@fanatid fanatid deleted the http-parser-port branch January 8, 2020 17:50
@fanatid fanatid mentioned this pull request Jan 10, 2020
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