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

efficient runtime version resolution #438

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cross19xx
Copy link

Why

My company had a custom app.config.js file which helped construct the environment variables based on the profile being used. As a debugging measure, console.log was introduced in this file. A stripped down version of this can be found here

const hasProfilePrefix = !!process.env.EAS_BUILD_PROFILE;
if (hasProfilePrefix) {
  const profilePrefix = `${process.env.EAS_BUILD_PROFILE?.toUpperCase()}_`;

  console.log(`Using profile: ${profilePrefix}`); // Notice this line

  Object.entries(process.env)
    .filter(([key]) => key.startsWith(profilePrefix))
    .forEach(([key, value]) => {
      process.env[key.slice(profilePrefix.length)] = value;
    });
}

Because of the std stream being used in this package, calculating the runtime version in the [CALCULATE_EXPO_UPDATES_RUNTIME_VERSION] returned all logs plus what is expected. E.g.

Using profile: preview
{"runtimeVersion": "1.0.0"}

This results in

[CALCULATE_EXPO_UPDATES_RUNTIME_VERSION] 
SyntaxError: Unexpected token 'U', "Using prof"... is not valid JSON
    at JSON.parse (<anonymous>)
    at resolveRuntimeVersionAsync (/Users/Comp/.npm/_npx/5e31c5d24a898acf/node_modules/@expo/build-tools/dist/utils/resolveRuntimeVersionAsync.js:24:43)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async resolveRuntimeVersionForExpoUpdatesIfConfiguredAsync (/Users/Comp/.npm/_npx/5e31c5d24a898acf/node_modules/@expo/build-tools/dist/utils/expoUpdates.js:110:36)
    at async /Users/Comp/.npm/_npx/5e31c5d24a898acf/node_modules/@expo/build-tools/dist/builders/android.js:64:16
    at async BuildContext.runBuildPhase (/Users/Comp/.npm/_npx/5e31c5d24a898acf/node_modules/@expo/build-tools/dist/context.js:107:28)
    at async buildAsync (/Users/Comp/.npm/_npx/5e31c5d24a898acf/node_modules/@expo/build-tools/dist/builders/android.js:63:47)
    at async runBuilderWithHooksAsync (/Users/Comp/.npm/_npx/5e31c5d24a898acf/node_modules/@expo/build-tools/dist/builders/common.js:12:13)
    at async Object.androidBuilder (/Users/Comp/.npm/_npx/5e31c5d24a898acf/node_modules/@expo/build-tools/dist/builders/android.js:24:16)
    at async buildAndroidAsync (/Users/Comp/.npm/_npx/5e31c5d24a898acf/node_modules/eas-cli-local-build-plugin/dist/android.js:41:12)

How

By collecting all std data, and splitting by the OS' new line character, we can pick the last string and safely parse this to give the correct runtime version

Test Plan

  • Setup a project with expo-updates installed
  • Create a custom app.config.js and place console.log statements in there
  • Confirm it accurately calculates the runtime version

Copy link
Member

@wschurman wschurman left a comment

Choose a reason for hiding this comment

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

I think a more general solution might be needed since the console.log could happen after the evaluation (and thus the last EOL-terminated string would be that) or some other dependency could emit a console.log. Something we've done in the past is to overwrite console.log during evaluation or app.config.js, which might be worth looking into.

Copy link
Member

@wschurman wschurman left a comment

Choose a reason for hiding this comment

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

One other alternative would be to somehow expose to the app.config.js module that it is being evaluated as part of a command that should only log JSON at the end, and that it shouldn't do any console.logs. Maybe an environment variable or something.

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.

2 participants