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 implicit return #1285

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

Conversation

sanvisser
Copy link

@sanvisser sanvisser commented Jul 9, 2024

Changes

  • I added an explicit return for the case where both if-statements evaluate to false. This explicit return of undefined does not change the current behavior, so I do not think any additional test coverage is necessary.
Recording.2024-07-09.121631.mp4

References

Testing

- [ ] This change adds unit test coverage
- [ ] This change adds integration test coverage
- [ ] This change has been tested on the latest version of the platform/language

Checklist

@frederikprijck
Copy link
Member

frederikprijck commented Jul 9, 2024

Thanks, we should be able to merge this with a few changes. But trying to understand one thing first ... Why should our TS compiler settings affect yours?

We have not set noImplicitReturns to true. Assuming that is done with reason, I'd expect this not to impact consuming projects that does set it to true as we ship js and d.ts files. Can you give some context?

In this case, I think setting noImplicitReturns to true and fix the problem makes sense. But I'd also argue it should not be needed. So I'd like to try and understand what's going on prior to merging this.

@frederikprijck
Copy link
Member

frederikprijck commented Jul 9, 2024

I gave this a try in a project using [email protected] and @auth0/[email protected] and have set "noImplicitReturns": true.I was able to include and use the SDK just fine without any compiler issue. Which was also concluded by @nandan-bhat here: #1280 (comment).

So I think we need a reproduction to investigate what is happening, rather than merge a PR that fixes something that we can not reproduce.

@sanvisser
Copy link
Author

So the problem was on my end. I imported the file wrong. Using the normal import fixes the problem too without needing a noImplicitReturns workaround. my bad!
image

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.

Implicit return in auth0-spa-js/src/cache /cache-manager.ts when TS "noImplicitReturns" is set to true
2 participants