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

Sourcemap of Cesium.js (iife) points to missing source files (v1.121.0) #12182

Closed
s3xysteak opened this issue Sep 4, 2024 · 6 comments · Fixed by #12263
Closed

Sourcemap of Cesium.js (iife) points to missing source files (v1.121.0) #12182

s3xysteak opened this issue Sep 4, 2024 · 6 comments · Fixed by #12263

Comments

@s3xysteak
Copy link
Contributor

What happened?

#11993 was fixed in v1.118 but it comes back in v1.121.0.

Sourcemap for "/foo/bar/node_modules/.pnpm/cesium@1.121.0/node_modules/cesium/Build/CesiumUnminified/Cesium.js" points to missing source files

Reproduction steps

  1. pnpm i
  2. pnpm dev

The warning will be throwed by vite server.

Sandcastle example

none

Environment

Browser: Edge
CesiumJS Version: v1.121.0
Operating System: windows 10

@jjspace
Copy link
Contributor

jjspace commented Sep 4, 2024

Thanks for catching this @s3xysteak
We should not normally be publishing source maps. I believe this slipped in after I ran an extra build command while troubleshooting some issues publishing 1.121. This should be addressed in 1.121.1 which was just published. I'll be opening a separate issue to start taking a look at some better release automation/checks so this sort of thing doesn't happen again shortly.
Please let me know if the new version doesn't fix this

@s3xysteak
Copy link
Contributor Author

Thank you for such a quick response! In my application, it has been fixed in v1.121.1 so I gonna close the issue.

@s3xysteak
Copy link
Contributor Author

@jjspace It came back again in v1.122.0 😇

@ggetz ggetz reopened this Oct 8, 2024
@jjspace
Copy link
Contributor

jjspace commented Oct 8, 2024

Thanks for bringing it up again @s3xysteak. Not really sure what the cause is, our release guide should be preventing this but maybe something's slipping through. We're not planning to do a minor release this time but this should be resolved on future releases.

@ggetz in addition to potentially reviewing the steps in the release guide I had some other thoughts for more automated prevention

The make-zip build process does not generate the sourcemaps at all but build does.
I see a couple options to fix/prevent this but not sure what's the best process wise?

  1. add /Build/**/*.js.map to the .npmignore so it never gets published
    • I think this would work but could also end up being a "band-aid" that bites us later if build and make-zip generate different JS files
  2. Update the release guide to say run npm run make-zip again right before the publish steps
  3. add a prepublishOnly script to the package.json to run gulp makeZip before every publish to guarantee it's the files we want
    • I think this is the best case? it makes the publish command a bit slower to wait for it to generate the docs and stuff but is probably the safest and most foolproof

Thoughts on some or all of the above changes?

CC @lukemckinstry since you're doing the next release

@ggetz
Copy link
Contributor

ggetz commented Oct 8, 2024

@jjspace I'm thinking the problem here is that we run some build steps as a part of the test command. Perhaps we should come up with a better strategy for dependent tasks.

In the meantime, I think 1 –

add /Build/**/*.js.map to the .npmignore so it never gets published

– is the most straightforward to prevent this particular issue from showing up. Since it's relatively simple, I don't see why we shouldn't do it.

@lukemckinstry
Copy link
Contributor

@jjspace it sounds like you have a solution for this, could you get it in by next Wednesday in advance of the next release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants