fix: prevent to load script again

[hosseinmd]

#749

Summary

In this PR, I prevented loadScript to load scripts is loaded before

Test plan

This is happening in remote federation with huge size of script and too many modules

[changeset-bot]

🦋 Changeset detected

Latest commit: 55cce05

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@callstack/repack	Patch
@callstack/repack-dev-server	Patch
@callstack/repack-init	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
repack-website	⬜️ Ignored (Inspect)	Visit Preview		Oct 16, 2024 0:28am

[jbroma]

Hey @hosseinmd, sorry for the late reply but I was unavailable most of the week.

The concept introduced in this PR looks great, we definitely want this in the next major (5.0) release!

Can you please add some tests for this scenarios? We need to document this behaviour as well as we can

I've also noticed that we're using the same variable
sriptsPromises for both prefetching and loading - while in theoery these should not overlap, I would go with creating a separate variables to avoid potential collision in rare edge cases.

We also need to address housekeeping of the promises that have already been fulfilled - we should remove the entires from scriptsPromises that are not relevant anymore.

Last thing that comes to my mind: perhaps we could return a promise in prefetchScript and loadScript instead of executing loadProcess again after awaiting the previous promise?

[hosseinmd]

Can you please add some tests for this scenarios? We need to document this behaviour as well as we can

Yes, I will do it

//updated
Done

[hosseinmd]

I've also noticed that we're using the same variable
sriptsPromises for both prefetching and loading - while in theoery these should not overlap, I would go with creating a separate variables to avoid potential collision in rare edge cases.

loadScript and prefetchScript are using same resolveScript. If we separate those, the same bug #749 possible to be happening.

[hosseinmd]

We also need to address housekeeping of the promises that have already been fulfilled - we should remove the entires from scriptsPromises that are not relevant anymore.

Ok, I will do it.

//Updated
Done

[hosseinmd]

I've also noticed that we're using the same variable
sriptsPromises for both prefetching and loading - while in theoery these should not overlap, I would go with creating a separate variables to avoid potential collision in rare edge cases.

loadScript and prefetchScript are using same resolveScript. If we separate those, the same bug #749 possible to be happening.

i added a test for that. 969d6d8

[hosseinmd]

Last thing that comes to my mind: perhaps we could return a promise in prefetchScript and loadScript instead of executing loadProcess again after awaiting the previous promise?

Done

[hosseinmd]

I've also noticed that we're using the same variable
sriptsPromises for both prefetching and loading - while in theoery these should not overlap, I would go with creating a separate variables to avoid potential collision in rare edge cases.

loadScript and prefetchScript are using same resolveScript. If we separate those, the same bug #749 possible to be happening.

In this ee3a58c commit loadScript will excute script if promise is for prefetch

[jbroma]

@hosseinmd can you merge newest main and resolve the conflicts? Then I will proceed with another review, thanks!

[hosseinmd]

Done

[jbroma]

Done

Nice job! I'll try to review it later today

[jbroma]

lets clean this up a little bit, but looks good overall 👍

[hosseinmd]

Done

[jbroma]

all looks good, just one last thing 🙏

[hosseinmd]

Last commits rebased

[jbroma]

LGMT 🎉

[jbroma]

Thank you for your work on this @hosseinmd, this was no easy issue to tackle! 👏 🚀