Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Streamline build processes with esbuild #10399
Streamline build processes with esbuild #10399
Changes from 41 commits
ff559c7
4bb8024
355dd42
54ef709
eb60fbf
f6cf110
bddf66e
e71c886
eb87e96
44b9610
0871e6a
27bbdbf
ec317da
16a9c80
7ac3fd0
d867651
290bed0
3938feb
4ecefb5
867ab1a
6404f52
1f99b62
018aca6
3293a5b
4cf8ba1
318c276
6a31cf1
8d7faae
312cb98
9905c12
1fbc968
213cdfc
6ae6cda
8bc8355
3a8e5c8
0a19ae3
095033d
17babe9
ad0dc45
24c4590
d6b9a96
c575a3f
7fd59fd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't Cesium now 100% require a bundler to use modules? (And a bundler that allows for node resolution rules at that). Or are we still embedding third party code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're embedding the third-party code in these builds. In the blog post I mention that production apps that use their own bundler should be using modules directly
Source
, which does not have third party code bundled. Which is what we've previously recommended as well if I understand correctly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have previously recommended it, but since we embedded ThirdParty code in source, you didn't need a bundler to use them. Now you do.
Another problem I just noticed is that
package.json
lists everything as a dev dependency, which may not be correct because that means if Inpm install Cesium
and then try to useSource
it's going to pull in modules that don't exist because they are not adependency
orpeerDependency
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, I see the distinction. For the point about the bundler, I will update both
CHANGES.md
and the blog post with the requirement.For the latter, I will
package.json
such that modules inSource/ThirdParty
are now marked as direct dependencies. Sound correct?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It's unfortunate because people who want to use the bundled Cesium.js still need to sync those dependencies but there's no other way that I know of to do it and have all usage-types work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey all, speaking of
package.json
, would this be a good time to address #9212 as well? It's really important to get the exports right, I had a workaround but had to get rid of it to be able to use another important dependency at all...ETA: to clarify, any resource that you intend for people to be able to bundle or extract separately (CSS, images, maybe data files like terrain height / IAU JSON) should have its own
exports
entry, and excluding those entries makes consumption of the resources effectively impossible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @thw0rted, thanks for pointing this out! We have some additional thoughts on related improvements beyond just this PR, including considering #9212. We'll add our thoughts or any updates to that issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be absolutely top of the list with 📣, it's not a breaking change and it's a huge deal.