Skip to content

Conversation

knervous
Copy link
Contributor

@knervous knervous commented Oct 1, 2025

Follow-up to the original PR #17175

The different issues pointed out by different people on the BJS forum post point to a caching issue - some scripts were loaded fresh while others were stale and cached by the browser. This led to undefined behavior all around and the page would be effectively "broken" depending on which assets were cached or not, i.e. anything could go wrong and wouldn't necessarily present as a predictable bug.

Previously there were duplicate names from the dist output and monaco worker output which is cached locally in a browser:
image

index.html does not have a Cache-Control response header and respects the client's request of max-age=0 - this is good. index.html should always fetch the latest.

It also appears there is a CDN purge script running on the Playground deployment script, although I don't have access to the CI yaml. https://dev.azure.com/babylonjs/ContinousIntegration/_build/results?buildId=44262&view=logs&j=275f1d19-1bd8-5591-b06b-07d489ea915a&t=4d2ea636-6e6e-5831-8b5a-8ce5e9ce8bbc
If that's the case for the last operation in that script, good. CDN purge handles the server cache.

Takeaway from the existing deployment pipeline and CDN architecture, nothing needs to change. If we can guarantee unique output from a build each time, there is no room for cache confusion from the client's browser. This only applies to the Playground and I haven't looked into other deployed apps. Cache-Control of 2 days is fine. Eventually this issue would've sorted itself out in the field but 2 days is a long time to wait with undefined/broken behavior.

The additional changes in this PR are consolidated in these two commits:

1e9c765
knervous@bd197e8

These changes ensure hashed output from every build and deploy for relevant bundled js files. The generated entrypoint for babylon.playground.[hash].js is injected into the index.html with the WebpackHTMLPlugin. The helper function for fetching scripts does append a query parameter to bust cache, but dependent chunks of scripts do not automatically follow the same convention. Ideally we can get rid of that ?t=[timestamp] and let caching do its business.

For anyone testing changes, please check https://playgroundv2.vercel.app which should be the exact hosted bundle set from the deployment build.

For sanity's sake here are three browsers on mobile tested with the current code: Edge, Firefox and Chrome

image image image

With editor open:

image

@knervous
Copy link
Contributor Author

This is the last of my tests that still isn't working:

In this PG:
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17216/merge/#SPHLNB#1
Go to the "test" file, rename the tata variable to something else: the occurrences in index.js won't be updated.

Noted - I need to do a pass on JS and local typings. Does the same thing work in TS?

Yes, it does work in TS!

Fixed here, no longer injecting ambient module declarations for local modules, was preventing updates from exports/imports.
9da714c

It doesn't work for me. Try in https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17216/merge/#SPHLNB#2

I think there may have been something related to during-development serializing of that project right there... I see the issue on that saved Playground, but when I start from scratch and save a new project and try the repro steps it seems to work every time. If you have the chance could you give that a shot? I will take a closer look into exactly what that case it doing there as well.

@knervous
Copy link
Contributor Author

Ditto above; this is the last of my tests that still isn't working.

The "Create New" button doesn't ask for confirmation anymore.

Is this still occurring when you don't have this option selected, or more specifically is this option doing what it's supposed to? That popup should only come up if this is selected if I'm reading and testing things right on the current PG

image

@Popov72
Copy link
Contributor

Popov72 commented Oct 14, 2025

This is the last of my tests that still isn't working:

In this PG:
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17216/merge/#SPHLNB#1
Go to the "test" file, rename the tata variable to something else: the occurrences in index.js won't be updated.

Noted - I need to do a pass on JS and local typings. Does the same thing work in TS?

Yes, it does work in TS!

Fixed here, no longer injecting ambient module declarations for local modules, was preventing updates from exports/imports.
9da714c

It doesn't work for me. Try in https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17216/merge/#SPHLNB#2

I think there may have been something related to during-development serializing of that project right there... I see the issue on that saved Playground, but when I start from scratch and save a new project and try the repro steps it seems to work every time. If you have the chance could you give that a shot? I will take a closer look into exactly what that case it doing there as well.

It still doesn't work for me. Try with https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17216/merge/?snapshot=refs/pull/17216/merge#TX2DRJ#2 (I've just created the PG)

@knervous
Copy link
Contributor Author

This is the last of my tests that still isn't working:

In this PG:
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17216/merge/#SPHLNB#1
Go to the "test" file, rename the tata variable to something else: the occurrences in index.js won't be updated.

Noted - I need to do a pass on JS and local typings. Does the same thing work in TS?

Yes, it does work in TS!

Fixed here, no longer injecting ambient module declarations for local modules, was preventing updates from exports/imports.
9da714c

It doesn't work for me. Try in https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17216/merge/#SPHLNB#2

I think there may have been something related to during-development serializing of that project right there... I see the issue on that saved Playground, but when I start from scratch and save a new project and try the repro steps it seems to work every time. If you have the chance could you give that a shot? I will take a closer look into exactly what that case it doing there as well.

It still doesn't work for me. Try with https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17216/merge/?snapshot=refs/pull/17216/merge#TX2DRJ#2 (I've just created the PG)

Ha, I just noticed your test module doesn't have any extension, it's just called test not test.js... The fallback in this case is default to JS language features and module compiling, but honestly think this one slipped through the cracks and just shouldn't be supported, i.e. all files should have an extension otherwise can't be parsed/part of the project's final result. There is more I want to do with different file extensions and syntax highlighting, embedding in the PG at a later date, would you be okay with getting this part of the work (preventing no extension on a filename) with that?

@Popov72
Copy link
Contributor

Popov72 commented Oct 15, 2025

Ha, I just noticed your test module doesn't have any extension, it's just called test not test.js... The fallback in this case is default to JS language features and module compiling, but honestly think this one slipped through the cracks and just shouldn't be supported, i.e. all files should have an extension otherwise can't be parsed/part of the project's final result. There is more I want to do with different file extensions and syntax highlighting, embedding in the PG at a later date, would you be okay with getting this part of the work (preventing no extension on a filename) with that?

That's totally fine with me, validating the PR!

@knervous
Copy link
Contributor Author

Looks like there are some conflicts, I will merge upstream and have changes pushed tomorrow morning.

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 16, 2025

Reviewer - this PR has made changes to one or more package.json files.

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 16, 2025

You have changed file(s) that made possible changes to the sandbox.
You can test the sandbox snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/SANDBOX/refs/pull/17216/merge/

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 16, 2025

You have made possible changes to the playground.
You can test the snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17216/merge/

The snapshot playground with the CDN snapshot (only when available):

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17216/merge/?snapshot=refs/pull/17216/merge

Note that neither Babylon scenes nor textures are uploaded to the snapshot directory, so some playgrounds won't work correctly.

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 16, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 16, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 16, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 16, 2025

@knervous
Copy link
Contributor Author

@ryantrem I merged in your most recent changes around inspectorv2 and validated state hydration for the cases I'm familiar with, would probably need a pass from your side to make sure it's all kosher

@ryantrem
Copy link
Member

@ryantrem I merged in your most recent changes around inspectorv2 and validated state hydration for the cases I'm familiar with, would probably need a pass from your side to make sure it's all kosher

Thanks for the heads up! I tested it and its not working correctly with older versions of Babylon. I think there was a bad merge and the changes from these two PRs did not get merged correctly into your branch:

You can test by using older versions of Babylon by doing the following:

  1. In Playground, click the version in the upper right and choose an older version.
  2. In Playground, add version=7.0.0 to the query params
  3. In Sandbox, add version=7.0.0 to the query params

For older versions, there should not be unhandled errors and if you try to enable Inspector v2 you should get an alert saying Inspector v2 can only be used with the latest Babylon.

Copy link
Member

@ryantrem ryantrem left a comment

Choose a reason for hiding this comment

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

Inspector v2 with old versions regressed. More info in this comment: #17216 (comment)

@knervous
Copy link
Contributor Author

@ryantrem I merged in your most recent changes around inspectorv2 and validated state hydration for the cases I'm familiar with, would probably need a pass from your side to make sure it's all kosher

Thanks for the heads up! I tested it and its not working correctly with older versions of Babylon. I think there was a bad merge and the changes from these two PRs did not get merged correctly into your branch:

You can test by using older versions of Babylon by doing the following:

  1. In Playground, click the version in the upper right and choose an older version.
  2. In Playground, add version=7.0.0 to the query params
  3. In Sandbox, add version=7.0.0 to the query params

For older versions, there should not be unhandled errors and if you try to enable Inspector v2 you should get an alert saying Inspector v2 can only be used with the latest Babylon.

Nice, thanks and sorry I missed that merge, was a lot of deviating points. I've got those changes in and tested against current PG and seems to function as intended, let me know if there's anything I missed

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 17, 2025

Reviewer - this PR has made changes to one or more package.json files.

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 17, 2025

You have made possible changes to the playground.
You can test the snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17216/merge/

The snapshot playground with the CDN snapshot (only when available):

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/17216/merge/?snapshot=refs/pull/17216/merge

Note that neither Babylon scenes nor textures are uploaded to the snapshot directory, so some playgrounds won't work correctly.

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 17, 2025

You have changed file(s) that made possible changes to the sandbox.
You can test the sandbox snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/SANDBOX/refs/pull/17216/merge/

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 17, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 17, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 17, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Oct 17, 2025

@ryantrem
Copy link
Member

ryantrem commented Oct 17, 2025

@ryantrem I merged in your most recent changes around inspectorv2 and validated state hydration for the cases I'm familiar with, would probably need a pass from your side to make sure it's all kosher

Thanks for the heads up! I tested it and its not working correctly with older versions of Babylon. I think there was a bad merge and the changes from these two PRs did not get merged correctly into your branch:

You can test by using older versions of Babylon by doing the following:

  1. In Playground, click the version in the upper right and choose an older version.
  2. In Playground, add version=7.0.0 to the query params
  3. In Sandbox, add version=7.0.0 to the query params

For older versions, there should not be unhandled errors and if you try to enable Inspector v2 you should get an alert saying Inspector v2 can only be used with the latest Babylon.

Nice, thanks and sorry I missed that merge, was a lot of deviating points. I've got those changes in and tested against current PG and seems to function as intended, let me know if there's anything I missed

I did a quick test of Playground, but it looks like Inspector v2 is not working at all. It looks like there are still merge issues on these lines:

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.

10 participants