repack

repack copied to clipboard

The TM provides the ability to provide a synchronous function call via JSI. Do we need these synchronous methods in the Re.Pack?

Here you can find an example API:

    loadScript(scriptId: string, config: Object): Promise<Object>;
    loadScriptSync(scriptId: string, config: Object): Object

Hence, the dev could use the sync or async method if needed.

Originally posted by @troZee in https://github.com/callstack/repack/pull/344#discussion_r1173638141

@troZee please update the description 🙏 thanks!

@jbroma done

Personally, I'm not sure whether sync API is useful as loadScript is called only for the async chunks right now.

The only thing that comes to mind where this would be helpful is that we could split the the main chunk into smaller chunks and allow them to be OTA updated. As all chunks would be required at startup, the sync API could prove useful there.

To me it looks like loadScript is the only sensible candidate to leverage the sync API

Typically a user should see a loading screen/indicator when a script is being fetched and evaluated. Having sync API for loading a script would only make sense for local chunks but then it would be a little quirky to decide whether to use sync or async method 🤔

Edit: on top of that, in the linked docs we discourage users from using local chunks if they are using Hermes because it might result in degraded performance.

I agree with @RafikiTiki, we would need to integrate it in a way that would allow for such optimisation in this rare case and keep async loadScript as default.

Yeah, keeping the async as the default sounds good! And since we have a technological possibility to implement a sync method let's do that – maybe someone will benefit from it in a way that's not obvious to us now.

@thymikee WDYT

Agree on keeping the default. How big of a maintenance investment would it be to implement, test and document the loadScriptSync method? Frankly I would wait for the opportunity to solve the real problem to arise before publishing the API and hoping for adoption

loadScript is used mostly internally, although it is exposed as a public API and can be used on it's own. I imagine we would have to add a field to Script config returned from resolver to turn on synchronous loading like this:

import { ScriptManager, Script } from '@callstack/repack/client';

ScriptManager.shared.addResolver(async (scriptId) => {
  // In development, get all the chunks from dev server.
  if (__DEV__) {
    return {
      url: Script.getDevServerURL(scriptId),
      cache: false,
    };
  }

  // In production, get chunks matching the regex from filesystem.
  if (/^.+\.local$/.test(scriptId)) {
    return {
      url: Script.getFileSystemURL(scriptId),
      sync: true   // <------- 
    };
  } else {
    return {
      url: Script.getRemoteURL(`https://my-domain.dev/${scriptId}`),
    };
  }
});

Maintenance wise, this is not part of something that changes often (or at all) from my experience, we can also consider marking it as experimental.

This issue has been marked as stale because it has been inactive for 30 days. Please update this issue or it will be automatically closed in 14 days.

Typically a user should see a loading screen/indicator when a script is being fetched and evaluated. Having sync API for loading a script would only make sense for local chunks but then it would be a little quirky to decide whether to use sync or async method 🤔

Edit: on top of that, in the linked docs we discourage users from using local chunks if they are using Hermes because it might result in degraded performance.

hey @RafikiTiki , do you have any results/ performance benchmarks that support this statement? For any super-app development, are there any guidelines around when/ how to choose certain parts of your app as Remote vs Local Chunks?

@jbroma please do add as well

This issue has been marked as stale because it has been inactive for 30 days. Please update this issue or it will be automatically closed in 14 days.

This issue has been marked as stale because it has been inactive for 30 days. Please update this issue or it will be automatically closed in 14 days.

This issue has been automatically closed because it has been inactive for more than 14 days. Please reopen if you want to add more context.