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

Bug in invalidateScripts leading to app crash #553

Open
sriharshaarangi opened this issue Apr 16, 2024 · 3 comments
Open

Bug in invalidateScripts leading to app crash #553

sriharshaarangi opened this issue Apr 16, 2024 · 3 comments
Labels
area:repack The issue is about logic/code inside of Re.Pack. type:bug A bug report.

Comments

@sriharshaarangi
Copy link

Environment

React Native android (but I believe the bug is not specific to RN android)

Description

Calling invalidateScripts for a chunk is causing a crash when next attempting to use that chunk. On debugging it, the chunk file is getting deleted, but the entry in sqlite db is not getting cleared. It looks like there is a bug in invalidateScripts method. resolveScripts method appends caller to the scriptId to get the cacheKey (const cacheKey = `${scriptId}_${caller ?? 'unknown'}`;), while invalidateScripts directly uses scriptId as the cacheKey.

Reproducible Demo

  1. Have a remote code split chunk.
  2. Call invalidateScripts with that chunk's scriptId
  3. Try using that chunk in UI, it leads to crash. com.facebook.react.common.JavascriptException: ChunkLoadError: Loading chunk <scriptId> failed
@jbroma jbroma added type:bug A bug report. area:repack The issue is about logic/code inside of Re.Pack. labels Apr 16, 2024
@jbroma
Copy link
Member

jbroma commented Apr 16, 2024

@jbroma I am having few queries before getting into the fix:

  1. caller Documentation says Name of the calling script - it can be for example: name of the bundle, chunk or container. However, the default value is main which is not the default bundle/chunk/container name.
  2. Should default caller value be made main instead of unknown?
  3. I am unable to grasp what value the caller is adding for cacheKey in the first place, if the underlying downloaded file is the same irrespective of the caller value (I am assuming so as per the downloaded chunk file name). Can you please help elaborate, or point me to any other documentation behind the design decision?
  1. & 2. The name main comes from webpack, when you declare entry with a string or an array, it defaults the entry chunk name to main. See webpack docs. So it's a decent default I think, but it's good to double check this if this is really the case.
  2. I've looked at it and I'm pretty sure the caller is there to create some sort of uniqueID, since scriptID is a string and can easily be duplicated in an app. Adding a caller creates a scope for it so it's harder to get a collision.

@jbroma
Copy link
Member

jbroma commented Apr 17, 2024

I suppose we could drop the caller altogether for the uniqueID, and rely on users specyfing output.uniqueName to avoid collisions.

EDIT: From my understanding, the scriptID is unique within a project, but there might be collision for example when using MF. If the configuration includes the output.uniqueName then we should be able to avoid any collision because the scriptID will be prefixed with uniqueName.

Copy link

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.

@github-actions github-actions bot added the Stale label May 18, 2024
@jbroma jbroma removed the Stale label May 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:repack The issue is about logic/code inside of Re.Pack. type:bug A bug report.
Projects
None yet
Development

No branches or pull requests

2 participants