Bug/reimplement headers

[jphenow]

Addresses #162

Open issue with axios test which is 500ing on header change. I don't know how to address the test but my notes may explain why the change would have broken something.

• Firefox's MDN docs spec for Headers is slightly different from lib.dom.d
  • Namely MDN specs entries, keys, and values functions
  • Weirdly, while the spec doesn't call out forEach it is available on Chrome and Firefox
  • Chrome's forEach value param lists values as an array of string, while Firefox's is a string of comma-separated values
  • node's implementation matches Firefox more closely in terms of forEach
  • Furthermore, lib.dom (which I believe matches Node's actual implementation) doesn't specify the entries, keys, or values functions, but does specify a forEach
• This implementation matches the lib.dom description and follows the Firefox/node spec of string, with comma separated values

Note: Values will only be comma separated automatically if using headers.append.

So, the open question is how I go about fixing the test that's broken. Also is there documentation, or anything that needs further update?

[mrkurt]

That was one of our old school, hard to understand tests. It was failing because the cookie_jar code we use expects headers.getAll. I added a more useful test in v8env.

[michaeldwan]

We just merged a big refactor (#152) into master which is causing those conflicts -- I'll rebase this PR and resolve them tonight or tomorrow.

[jphenow]

I got it running again with the reboot, but test output is a little more chatty with server output. Will have to dig in some more, but they look like tests related to Cookies as Kurt pointed out a few days ago, so not surprising failures at all. The header API is a slightly different from previously, so that's to be expected.

[michaeldwan]

Yeah the test output is way too verbose now since the e2e tests are using the local server which prints to STDOUT. We'll be looking into that soon.

[mrkurt]

@jphenow I think if the header just implements getAll, and maybe marks it deprecated like MDN, the cookie stuff will work fine.

[mrkurt]

Also the test reporter in CI gives much nicer output than the console: https://dev.azure.com/flydotio/fly/_build/results?buildId=59&_a=summary&tab=ms.vss-test-web.test-result-details&view=ms.vss-test-web.test-result-details

[jphenow]

Yea that's what I arrived at locally - just haven't had a chance to push and see what happens. I figure the lib.dom.d can present an API that's generally available and you can manually import the type if you want the added API bits. At least this way it'll be a superset rather than wholly different from lib.dom.d which is nice.

I did run into some other strange issues, so may pair at some point and see if we can improve some more of the internals here, but we'll see what happens after I do what I said I would in paragraph 1 😄

[jphenow]

Added back for..of Iterator support, updated external use of iterator to for..of and now it implements the Header interface.

Implementing Header interface meant that the class needed to be renamed (since I can't rename the global within scope, I don't believe), which also meant I had to assert type to FlyHeaders in order to get for..of since Iterator isn't specified in the lib.dom interface. Sort of awkward without controlling that interface or at least taking some control of it.

[michaeldwan]

Okay I see now, Headers in lib.dom doesn't support iteration, you need to include lib.dom.iterable for that. We'll need to add that to v8env's tsconfig then you don't need to cast. That said, I think it's better for v8env code to explicitly import modules instead of relying on the global declarations, so your change to FlyHeaders is great.

[jphenow]

Alrighty. Pushed some updates, sorry for slowness getting this one wrapped up. Pretty sure there's still some wonkiness going on. Will take another look this evening hopefully

[jphenow]

Well, those are weird

[mrkurt]

Wow that is really strange.

[michaeldwan]

I tracked down the strange errors to some bugs with how iterators + generators were implemented. I added some more tests and fixed some issues I uncovered, so we should be good now 😸

[jphenow]

[jphenow]

We cool to close this then? At long last?

[michaeldwan]

🎉 thanks @jphenow!

[jphenow]

Wow was I sleeping at the wheel? Thanks for reading through the changes 🤦‍♂️

[michaeldwan]

It's all good 😄