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

[Web] Fix: expo-modules-core server side error #28764

Merged
merged 5 commits into from
May 14, 2024
Merged

Conversation

alantoa
Copy link
Contributor

@alantoa alantoa commented May 10, 2024

Why

Hey, @tsapeta. I noticed that the expo-modules-core recently underwent some changes and is now broken on Next.js.

The reason for this issue is that we are importing SharedObject and NativeModule globally on the server side. Therefore, I have come up with a simple solution to fix the export of NativeModule and SharedObject.

But I'm not a big fan of this change because since we have already exported these global variables in './web/index', it feels like these NativeModule and SharedObject are being duplicated on web, which is a bit unnecessary.

How

Add a precondition for NativeModule and SharedObject: if they are in a server-side environment, simply export an empty object.

Test Plan

I have tested our project and created a minimal reproducible example locally. If you want, I can show you this example. However, I don't think it's necessary because this change doesn't break anything.

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label May 10, 2024
@alantoa
Copy link
Contributor Author

alantoa commented May 10, 2024

Let me know if anything else needs to be updated or if you have any feedback!

@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels May 10, 2024
Copy link
Member

@tsapeta tsapeta left a comment

Choose a reason for hiding this comment

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

Hey Alan, thanks for the PR!
Ideally if we don't have to do these checks. In theory it should be polyfilled on the server side, by src/web/CoreModule.ts, right?

@alantoa
Copy link
Contributor Author

alantoa commented May 12, 2024

Hey @tsapeta, yeah, that makes sense. So now the problem is clear: The issue lies in the import statement in /web/index.web.ts not working because it actually requires all variables to be exported from CoreModule.ts.

I have just updated my code and tested my example, and it is fixed. Could you please check it again when you have some free time?

@@ -1 +1 @@
import './CoreModule';
export * from './CoreModule';
Copy link
Member

Choose a reason for hiding this comment

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

Now I'm wondering why does changing import to export change the behavior?
Isn't it that we just need to import web/index before NativeModule and SharedObject are imported?

Copy link
Contributor Author

@alantoa alantoa May 14, 2024

Choose a reason for hiding this comment

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

Isn't it that we just need to import web/index before NativeModule and SharedObject are imported?

this change doesn't actually work.

The core reason is due to globalThis, which actually differs between Node.js and the browser.

On the Next.js server side (Node.js), it refers to global.
In the browser environment, it refers to window.

During SSR, the code may be executed multiple times, and the state of globalThis may not be persisted.

For example:
If I change the CodeModule.js code to the code below, it will work because the globalThis state can be persisted.

if (typeof window !== "undefined") {
    window.expo = {      
        EventEmitter,
        ...
    }
  } else if (typeof global !== "undefined") {
    global.expo = {
        EventEmitter,
        ...
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm now simply changing the import order to fix it:

When I execute import './web/index.js' on the server side, the module ./web/index.js is loaded first. Since ./web/index.js directly exports all content from ./CodeModules.js, this means that ./core/index.js will be executed and will correctly set globalThis.expo to { NativeModule, ....}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lmk if you have any questions!

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, though I still wonder if moving this import 👇 wouldn't be sufficient to solve the issue?

import './web/index';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, it doesn't work.

Copy link
Member

Choose a reason for hiding this comment

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

Could you please revert updating the submodule? 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just reverted it. Could you please take a look again?

Copy link
Member

@tsapeta tsapeta left a comment

Choose a reason for hiding this comment

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

Looks good now, thanks for fixing this! We'll publish it for SDK 51 in a few days.

@tsapeta tsapeta changed the title [Web]Fix: expo-modules-core server side error [Web] Fix: expo-modules-core server side error May 14, 2024
@tsapeta tsapeta merged commit 79b9c9a into expo:main May 14, 2024
10 of 12 checks passed
@alantoa
Copy link
Contributor Author

alantoa commented May 14, 2024

thank you! 🫡

@brentvatne brentvatne added the published Changes from the PR have been published to npm label May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants