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

Use polymorphic variants for some Request properties #68

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tom-sherman
Copy link
Contributor

@tom-sherman tom-sherman commented Dec 21, 2021

Relates to #30 and #38

Changes mode, credentials, cache, and redirect to use polymorphic variants.

I've left destination and referrerPolicy untouched for now as they have some problems. See #38 (comment)

~mode=?map(encodeRequestMode, mode),
~credentials=?map(encodeRequestCredentials, credentials),
~cache=?map(encodeRequestCache, cache),
~redirect=?map(encodeRequestRedirect, redirect),
Copy link
Owner

Choose a reason for hiding this comment

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

I had no idea this =?map syntax was valid 🤨 this was just how it converted from the original OCaml. Looks like it's a convenient way to pass an optional function return value as an optional method argument 🤔

Learn something new every day, I guess!

@@ -1,6 +1,6 @@
module AbortController = Webapi__Fetch__AbortController

/** Alias for anyone upgrading */
/* * Alias for anyone upgrading */
Copy link
Owner

Choose a reason for hiding this comment

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

The lack of ReScript support for docs comments is annoying. Maybe we should just delete this comment now 🤷 and arguably the alias, too, since this will be going onto the 2.0 branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to delete the aliases in a followup PR when all of these are merged as I don't fancy like dealing with the conflicts 😆

Copy link
Collaborator

@spocke spocke left a comment

Choose a reason for hiding this comment

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

It's a little bit annoying that you need to quote the polymorphic variants when it has a dash in them but I guess that is nothing we can do about except maybe get the compiler to support that syntax.

@tom-sherman
Copy link
Contributor Author

@spocke Yeah it looks quite awkward. I think another way of improving that situation is through IDE autocompletion too.

@TheSpyder TheSpyder force-pushed the main branch 5 times, most recently from fe0f2e9 to 7f2cf0a Compare May 22, 2024 12:41
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.

3 participants