Skip to content

feat: implement (basic) webidl & headers comments#1495

Merged
mcollina merged 4 commits intonodejs:mainfrom
KhafraDev:webidl
Jun 18, 2022
Merged

feat: implement (basic) webidl & headers comments#1495
mcollina merged 4 commits intonodejs:mainfrom
KhafraDev:webidl

Conversation

@KhafraDev
Copy link
Copy Markdown
Member

Fixes #933

WebIdl is pretty cool and it's used rather heavily in the fetch spec. Implementing a subset of it (that we need) will make everything much easier once it's finished. I can explain how it'll help if you want, just don't want to write out an essay if you don't care lol

I only did the Headers class as a PoC and in the process discovered a load of bugs and decided just to add the spec comments since I was already adding a ton to it and fixing stuff anyways 😄.

p.s.: it's very clear that HeadersList was not supposed to be Map-like structure, as each key can be duplicated and the actual appending doesn't occur until you use .get(...). Not a big deal, just something I noticed when working on it.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 14, 2022

Codecov Report

❌ Patch coverage is 97.22222% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.78%. Comparing base (8e9d902) to head (7bd99dc).
⚠️ Report is 1952 commits behind head on main.

Files with missing lines Patch % Lines
lib/fetch/util.js 90.90% 1 Missing ⚠️
lib/fetch/webidl.js 94.11% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1495   +/-   ##
=======================================
  Coverage   94.78%   94.78%           
=======================================
  Files          49       50    +1     
  Lines        4293     4335   +42     
=======================================
+ Hits         4069     4109   +40     
- Misses        224      226    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@KhafraDev KhafraDev requested a review from Ethan-Arrowood June 15, 2022 01:14
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Could you add some tests for the new code?

Comment thread lib/fetch/webidl.js
Co-authored-by: Matteo Collina <matteo.collina@gmail.com>
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Could you add some tests for the new code?

@KhafraDev
Copy link
Copy Markdown
Member Author

KhafraDev commented Jun 15, 2022

added tests

all new stuff should be tested with existing tests already (everything passed other than Symbol tests which shouldn't have worked), just added some tests for invalid/valid characters.

@KhafraDev KhafraDev requested a review from mcollina June 16, 2022 00:54
Copy link
Copy Markdown
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 3dec08b into nodejs:main Jun 18, 2022
@KhafraDev KhafraDev deleted the webidl branch June 18, 2022 14:50
KhafraDev added a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022
metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Dec 26, 2022
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
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.

Add spec comments to Header class

4 participants