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

[Meteor 3] Calling WebApp.addRuntimeConfigHook breaks __meteor_runtime_config__ #12939

Open
zarvox opened this issue Dec 31, 2023 · 3 comments · May be fixed by #13156
Open

[Meteor 3] Calling WebApp.addRuntimeConfigHook breaks __meteor_runtime_config__ #12939

zarvox opened this issue Dec 31, 2023 · 3 comments · May be fixed by #13156
Labels
Meteor 3 relates to Meteor 3 Type:Bug
Milestone

Comments

@zarvox
Copy link
Contributor

zarvox commented Dec 31, 2023

Affected versions: 3.0-rc.0 and 3.0-beta.0 are impacted (and presumably the other 3.0 alphas are all impacted too). Meteor 2.14 is not.

OS: all (verified on macOS 14.2.1 and Fedora Linux 39)

Reproduction repository: https://github.com/zarvox/meteor-trivial-runtime-config-hook

Steps to reproduce:

  1. Use a meteor-3.0 alpha or beta or rc.0
  2. Make any call to WebApp.addRuntimeConfigHook() in server code. The hook can be an empty function; it doesn't matter what it is.
  3. Run the app (meteor)
  4. attempt to load the page at http://localhost:3000
  5. observe a blank white page and errors on the JS console

I believe what's happening is that packages/webapp/webapp_server.js calls runtimeConfig.hooks.forEach, and Hook defaults to binding the Meteor environment, which results in wrapping whatever callback the user provided in Meteor.bindEnvironment which seems to return a Promise under meteor 3. Then, when rendered as a string, we get [object Promise] rather than the intended result of running the hook function, which gets propagated into the line

  <script type="text/javascript">__meteor_runtime_config__ = JSON.parse(decodeURIComponent([object Promise]))</script>

which is a syntax error. Later, other parts of the meteor runtime expect that variable to be defined, and those error out too.

I'm not particularly familiar with the expected/intended behavior of bindEnvironment in the fiberless world, but if that's behaving as designed, then we probably want something like the following patch in packages/webapp/webapp_server.js to actually await each hook's completion:

diff --git a/packages/webapp/webapp_server.js b/packages/webapp/webapp_server.js
index 1245207519..3e79976618 100644
--- a/packages/webapp/webapp_server.js
+++ b/packages/webapp/webapp_server.js
@@ -425,10 +425,10 @@ WebApp.addRuntimeConfigHook = function(callback) {
   return runtimeConfig.hooks.register(callback);
 };
 
-function getBoilerplateAsync(request, arch) {
+async function getBoilerplateAsync(request, arch) {
   let boilerplate = boilerplateByArch[arch];
-  runtimeConfig.hooks.forEach(hook => {
-    const meteorRuntimeConfig = hook({
+  await runtimeConfig.hooks.forEachAsync(async hook => {
+    const meteorRuntimeConfig = await hook({
       arch,
       request,
       encodedCurrentConfig: boilerplate.baseData.meteorRuntimeConfig,
@zarvox
Copy link
Contributor Author

zarvox commented Apr 21, 2024

This remains unfixed as of 3.0-rc.0.

@nachocodoner
Copy link
Member

Taking a note, we will consider to fix on next RC. Thank you

@nachocodoner nachocodoner added the Meteor 3 relates to Meteor 3 label Apr 25, 2024
@nachocodoner nachocodoner linked a pull request May 21, 2024 that will close this issue
1 task
@StorytellerCZ StorytellerCZ linked a pull request May 22, 2024 that will close this issue
1 task
@nachocodoner
Copy link
Member

nachocodoner commented May 22, 2024

We have included a fix on the next PR, which will be included on the upcoming RC. A proper test scenario has been added to cover this untested behavior.

#13156

As a reminder I add here the docs on how to use this hook.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Meteor 3 relates to Meteor 3 Type:Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants