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

chore: removed irrelevants | undefined #2524

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

Conversation

Iuriiiii
Copy link
Contributor

Just fix of few typos, nothing else.

@Iuriiiii
Copy link
Contributor Author

Tests on Ubuntu WSL passed without problems.

@rschristian
Copy link
Contributor

rschristian commented Jun 18, 2024

These are not (all) irrelevant, new(-er) versions of TS support differentiating between optional params (?) and those that can be provided as undefined, for example: foo() vs foo(undefined)

TS Docs

@Iuriiiii
Copy link
Contributor Author

These are not (all) irrelevant, new(-er) versions of TS support differentiating between optional params (?) and those that can be provided as undefined, for example: foo() vs foo(undefined)

TS Docs

I didn't know about that, that seems more relevant on object members than function parameters. even otherwise is recomended (i think) to avoid developers to write undefined as arguments in functions

@rschristian
Copy link
Contributor

Certainly has some varying degrees of usefulness, true, but foo?: type | undefined follows JS semantics which some of your changes now disallow.

There's no real recommendation that I know of to disallow undefined function params as that's fundamentally indistinguishable from an unprovided param at runtime.

@Iuriiiii
Copy link
Contributor Author

Iuriiiii commented Jun 19, 2024

Certainly has some varying degrees of usefulness, true, but foo?: type | undefined follows JS semantics which some of your changes now disallow.

There's no real recommendation that I know of to disallow undefined function params as that's fundamentally indistinguishable from an unprovided param at runtime.

If exactOptionalPropertyTypes is not setted up to true within the deno.json file (the current case) developers will be able to use undefined as arguments if they want

@rschristian
Copy link
Contributor

If exactOptionalPropertyTypes is not setted up to true within the deno.json file (the current case) developers will be able to use undefined as arguments if they want

...and if exactOptionalPropertyTypes is enabled, they no longer can pass undefined as args, thereby your changes here significantly change some APIs (from TS, anyhow).

Anyways, just something to be aware of.

@Iuriiiii
Copy link
Contributor Author

If exactOptionalPropertyTypes is not setted up to true within the deno.json file (the current case) developers will be able to use undefined as arguments if they want

...and if exactOptionalPropertyTypes is enabled, they no longer can pass undefined as args, thereby your changes here significantly change some APIs (from TS, anyhow).

Anyways, just something to be aware of.

You're right, just for this I checked for undefined as arguments within the project, the only place where undefined is being used as arguments is on the docs (.md files), and 2 test files (it won't break thought, the types that allow those undefineds is unknown).

The line on docs that's using undefined as argument is:

return ctx.render(undefined);

I just want to leave a note, reject this PR seems razonable if someone is really worried about a possible update of the Typescript settings, if that happens please remember to add | undefined to src\context.ts:84 to allow undefined as argument on this method.

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