Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Mar 23, 2023

Split out from #19028.

As of #18443, USE_SDL defaults to zero which means we don't want to include
library_sdl.js by default, even when AUTO_JS_LIBRARIES is set.

In order to make closure happy without library_sdl.js but with INCLUDE_FULL_LIBRARY
I had to add a closure type annotation for the Module.ctx object. I'm really not sure
why this was needed now but not before.

@sbc100 sbc100 changed the title Always include library_sdl.js and library_glfw.js conditionally Always include library_sdl.js and library_glfw.js conditionally. NFC Mar 23, 2023
@sbc100 sbc100 requested a review from kripken March 23, 2023 22:42
@sbc100 sbc100 force-pushed the conditional_js_libs branch from f6996fe to fe2c759 Compare March 23, 2023 23:15
@sbc100 sbc100 requested review from dschuff and juj and removed request for juj March 23, 2023 23:18
@sbc100 sbc100 force-pushed the conditional_js_libs branch from fe2c759 to c328cc4 Compare March 23, 2023 23:52
@sbc100 sbc100 enabled auto-merge (squash) March 24, 2023 00:04
Copy link
Member

@dschuff dschuff left a comment

Choose a reason for hiding this comment

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

I'm all for not having libraries magically included if not used; but we should probably have a changelog for this, right? Since this will break people who aren't using -lsdl or -lglfw if they should?

@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 24, 2023

I'm all for not having libraries magically included if not used; but we should probably have a changelog for this, right? Since this will break people who aren't using -lsdl or -lglfw if they should?

For glfw there is no change at all since USE_GLFW=2 is still the default.

For USE_SDL, we changes the default to 0 back in #18443 so users have to opt into using SDL already.

In other words I would argue this is very close to NFC.

@dschuff
Copy link
Member

dschuff commented Mar 24, 2023

oh, they need to USE_SDL=1 to get the SDL C headers?

@sbc100
Copy link
Collaborator Author

sbc100 commented Mar 24, 2023

oh, they need to USE_SDL=1 to get the SDL C headers?

Yes, as of #18443 that is needed. Users who don't specify USE_SDL will see #error "To use the emscripten port of SDL use -sUSE_SDL or -sUSE_SDL=2.

@sbc100 sbc100 force-pushed the conditional_js_libs branch from c328cc4 to 7a0515c Compare March 24, 2023 05:02
Copy link
Collaborator

@juj juj left a comment

Choose a reason for hiding this comment

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

Looks nice overall.

@sbc100 sbc100 merged commit 978e102 into main Mar 24, 2023
@sbc100 sbc100 deleted the conditional_js_libs branch March 24, 2023 16:50
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.

4 participants