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

Add autocomplete to XFrameOptionsOptions #322

Merged
merged 1 commit into from
Aug 18, 2021
Merged

Conversation

louy2
Copy link
Contributor

@louy2 louy2 commented Jul 6, 2021

Changes the XFrameOptionsOptions to let typescript suggest "DENY" and
"SAMEORIGIN" in autocomplete with the trick in
microsoft/TypeScript#29729

Changes the XFrameOptionsOptions to let typescript suggest "DENY" and
"SAMEORIGIN" in autocomplete with the trick in
microsoft/TypeScript#29729
@EvanHahn
Copy link
Member

EvanHahn commented Jul 7, 2021

Thanks for the pull request. It might be a few weeks before I get to it, but it's on my list.

@EvanHahn
Copy link
Member

Thanks for your work here.

I know that some people have these options as configuration files, so they want to pass strings and not more specific types. Does this still work if you pass a variable of type string, or does it now have to be one of those two options?

Also, if we do this, I think it's worth adding some kind of explainer comment.

(I'm away from my computer at the moment, otherwise I'd test this myself!)

@louy2
Copy link
Contributor Author

louy2 commented Jul 16, 2021

Does this still work if you pass a variable of type string

Yes, at type check this is equivalent to just string. The (string & {}) is to prevent the type checker from collapsing the outer union into just string, therefore losing the literals and disabling the autocomplete. But because string is a subtype of {} (which is the type of all non-nullish values), the intersection is equivalent to string, and thus the whole union is equivalent to string.

@louy2
Copy link
Contributor Author

louy2 commented Jul 16, 2021

This is also equivalent to LiteralUnion in sindresorhus/type-fest, but I was not sure about pulling in a whole dependency just for this. If you find other parts of type-fest useful, maybe that's the way to go.

I agree about adding comments, but not sure how detailed it should be.

@louy2
Copy link
Contributor Author

louy2 commented Jul 16, 2021

There is a rule in typescript-eslint that bans {} due to the confusion it may cause when not used within an intersection, so type-fest uses a longer form to work around that.

@EvanHahn
Copy link
Member

EvanHahn commented Jul 18, 2021 via email

@EvanHahn
Copy link
Member

EvanHahn commented Aug 1, 2021

I haven't forgotten about this but haven't had time to look at it yet. It's still on my list!

@EvanHahn EvanHahn changed the base branch from main to louy2-main August 18, 2021 01:12
@EvanHahn EvanHahn merged commit 24a1472 into helmetjs:louy2-main Aug 18, 2021
EvanHahn pushed a commit that referenced this pull request Aug 18, 2021
@EvanHahn
Copy link
Member

I made a couple of small changes and merged this in 2f13523.

As before, let me know if you'd like to be added to the contributors page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants