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

ESM Preload Fix #7161

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

ESM Preload Fix #7161

wants to merge 4 commits into from

Conversation

marklundin
Copy link
Member

This PR fixes playcanvas/editor#1236 along with https://github.com/playcanvas/editor-ui/pull/460.

This removes the ?hash= appended to ESM paths during preloading. This ensures it matches the cache key used for explicit imports in user code, resulting in the same module.

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@marklundin marklundin requested a review from a team December 2, 2024 15:17
@marklundin marklundin self-assigned this Dec 2, 2024
mvaligursky
mvaligursky previously approved these changes Dec 2, 2024
Copy link
Contributor

@mvaligursky mvaligursky left a comment

Choose a reason for hiding this comment

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

do we need to cherry pick to main_v1 as well?

@marklundin
Copy link
Member Author

Yep we do

@Maksims
Copy link
Collaborator

Maksims commented Dec 3, 2024

By removing hash, this will lead to lack of cache invalidations. This will break what developers usually do.
Currently, everything under files/assets folder can be hard cached, because assets pipeline will ensure hash is generated if file changes. So url with hash is hard cached. This is the case for all assets, including scripts.

By changing this, developers will have do more sophisticated rules on caching, and won't be able to use hard caching for scripts.

This is undesirable. Please do not change such well working rule.

The correct solution to this problem is to figure out how to ensure user imported script urls are pre-processed correctly, so they match.

@marklundin
Copy link
Member Author

Could you clarify? The hash param is not included in the classic scripts preload either, so this better aligns the two

@mvaligursky mvaligursky dismissed their stale review December 3, 2024 11:23

due to comment from Maksims

@Maksims
Copy link
Collaborator

Maksims commented Dec 3, 2024

Could you clarify? The hash param is not included in the classic scripts preload either, so this better aligns the two

I've just checked, and you are correct, there is no hash. Has this been changed at any point? As there was a rule: every asset within assets/files used hashes, so it could be hard cached (200 local storage). Everything outside of that folder is soft-cached (304).

@marklundin
Copy link
Member Author

Yep, script assets do not seem to have a file hash. See 7fe92ef#diff-59cbf44184755e9db171fb7873b5b92241e4bc05bffd75d001d9e6f738c38700R138-R141

The benefits around having a hash are valid though, but need further investigation

@marklundin
Copy link
Member Author

Just to follow up; this needs to be checked with the editor build/export process

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.

ESM Module Identity
3 participants