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

[FIX] Ensure consistent function return types #4153

Merged
merged 5 commits into from
Apr 1, 2022
Merged

Conversation

willeastcott
Copy link
Contributor

There are several places in the codebase where a function has inconsistent return types. These problems are spotted by the ESLint rule consistent-return. This PR attempts to fix these problems with a view to enabling this rule in the future.

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@willeastcott
Copy link
Contributor Author

Remaining problems that still need fixing:

image

@kungfooman
Copy link
Collaborator

Should the build step replace return undefined; with return; again to save the bytes? Probably doesn't matter too much after zipping. This would also remove some bytes from these existing cases:

image

Another option that helps with detecting this is to enable "strictNullChecks": true in jsconfig.json, for example:

image

@willeastcott
Copy link
Contributor Author

Yeah, I think such a micro-optimization is probably not worth it. 😄 I'll take a look at the strictNullChecks setting - thanks!

@@ -1471,11 +1471,11 @@ class Application extends EventHandler {
* @returns {object} A object containing the values calculated to use as width and height.
*/
resizeCanvas(width, height) {
if (!this._allowResize) return; // prevent resizing (e.g. if presenting in VR HMD)
if (!this._allowResize) return undefined; // prevent resizing (e.g. if presenting in VR HMD)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these should be null instead of undefined .. as the return type is object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly. I'm mainly just trying to keep the functionality identical and satisfy ESLint.

Debug.error(`batch group with id ${id} already exists`);
return;
Debug.error(`Batch group with id ${id} already exists.`);
return undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it could return this._batchGroups[id] so that the code would still work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, it'd be changing functionality. I'd rather address that separately.

@willeastcott willeastcott merged commit a0ddc12 into main Apr 1, 2022
@willeastcott willeastcott deleted the consistent-return branch April 1, 2022 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants