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.

Sign up for GitHub

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

Jump to bottom

[Web] Fix closure compiler typedef annotation #91202

Merged
merged 1 commit into from
Apr 29, 2024
Merged

[Web] Fix closure compiler typedef annotation #91202

merged 1 commit into from
Apr 29, 2024

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Apr 26, 2024 •
edited
Loading

The typedef annotation is expected to come bofre a var (or const) since it's most commonly used in externs. Use an inline definition instead.

@Faless Faless requested a review from a team as a code owner April 26, 2024 07:49
@Faless Faless mentioned this pull request Apr 26, 2024
Closed
@Faless Faless force-pushed the web/fix_cc_typedef branch from 5d89161 to 0d6fd03 Compare April 26, 2024 08:02
@akien-mga akien-mga added this to the 4.3 milestone Apr 26, 2024
@akien-mga akien-mga added cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release labels Apr 26, 2024
@akien-mga
Copy link
Member

akien-mga commented Apr 26, 2024 •
edited
Loading

This doesn't seem compatible with the JS file structure we have in 3.x, could you make a backport for that branch?

Note that you need latest 3.x (after #91151) to be able to get this far into the compilation and run into #91161.

@Faless
Copy link
Collaborator Author

Faless commented Apr 26, 2024

This doesn't seem compatible with the JS file structure we have in 3.x, could you make a backport for that branch?

I don't think the 3.x branch should be affected by this specifically. The bogus annotation is not there, so it should compile. If it fails, it should have a different error.

I'm trying to make a build now.

@akien-mga
Copy link
Member

Here's the build log on 3.x: #91151 (comment)

There's indeed another warning. It's a bit weird that warnings cause the build to fail, without explicit mention that warnings are treated as errors.

@Faless
Copy link
Collaborator Author

Faless commented Apr 26, 2024

There's indeed another warning. It's a bit weird that warnings cause the build to fail, without explicit mention that warnings are treated as errors.

Indeed, it does seems the warnings were just a red herring and the error is still there. Not sure why it worked for me at some point.

@Faless Faless marked this pull request as draft April 26, 2024 09:03
@akien-mga
Copy link
Member

I guess we can merge this as just a warning fix now.

@akien-mga akien-mga changed the title [Web] Fix JS typedef annotation causing build failure [Web] Fix JS typedef annotation warning Apr 26, 2024
@akien-mga akien-mga marked this pull request as ready for review April 26, 2024 09:33
@Faless Faless force-pushed the web/fix_cc_typedef branch from 0d6fd03 to 81c8508 Compare April 26, 2024 11:11
@Faless
[Web] Fix closure compiler typedef annotation
8079cd4 
The typedef annotation is expected to come bofre a var (or const) since
it's most commonly used in externs. Use an inline definition instead.
@Faless Faless force-pushed the web/fix_cc_typedef branch from 81c8508 to 8079cd4 Compare April 26, 2024 11:12
@Faless Faless changed the title [Web] Fix JS typedef annotation warning [Web] Fix closure compiler typedef annotation Apr 26, 2024
Faless
Faless commented Apr 26, 2024
View reviewed changes
platform/web/js/engine/features.js
@@ -72,8 +72,7 @@ const Features = { // eslint-disable-line no-unused-vars
*
* @returns {Array<string>} A list of human-readable missing features.
* @function Engine.getMissingFeatures
* @typedef {{ threads: boolean }} SupportedFeatures
* @param {SupportedFeatures} supportedFeatures
* @param {{threads: (boolean|undefined)}} supportedFeatures
Copy link
Collaborator Author

@Faless Faless Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if we should rename the parameter to requestedFeatures

Copy link
Member

Choose a reason for hiding this comment

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

@adamscott

@akien-mga akien-mga merged commit 8211b0d into godotengine:master Apr 29, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga removed cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Apr 29, 2024
@Faless Faless deleted the web/fix_cc_typedef branch April 29, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga approved these changes

Assignees
No one assigned
Labels
bug platform:web topic:buildsystem topic:porting
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
2 participants
@Faless @akien-mga