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

Fix remaining issues that require special Webpack configuration settings? #4876

Closed
markerikson opened this Issue Jan 18, 2017 · 7 comments

Comments

Projects
None yet
6 participants
@markerikson
Contributor

markerikson commented Jan 18, 2017

Per https://cesiumjs.org/2016/01/26/Cesium-and-Webpack/ , Cesium requires two specific Webpack configuration options to be set:

  • output.sourcePrefix : '' is needed because Cesium has multi-line strings, and Webpack tries to add tabs to them
  • module.unknownContextCritical: false is needed to avoid warnings from Cesium pulling in some third-party AMD-formatted modules like Knockout.

Unfortunately, this means that Cesium doesn't work out of the box with a project created using the Create-React-App tool, which hides the Webpack config options unless you run a one-time eject command to copy them to your actual project.

It would be nice if things could be cleaned up enough that Cesium could work with Webpack, with no addtiional config options needed. (The "copy Cesium files to /public" step is still a bit annoying, but acceptable.)

I imagine the multi-line strings would be fixable by either using ES6 template strings, or choosing alternate methods of string construction. I'm not sure how many there are in the Cesium codebase, or how much effort it would be to rework them.

Also not sure how difficult it would be to get the dynamic module warnings cleaned up, on either the Cesium side or Webpack side.

/cc @gaearon , @TheLarkInn for any thoughts they might have.

@mramato

This comment has been minimized.

Show comment
Hide comment
@mramato

mramato Jan 25, 2017

Member

Ultimately we want to make Cesium work out of the box with all of the most popular build solutions, which is why we'll most likely move to ES6 modules sometime this year. I expect there may be some Webpack specific issues, but the goal would be to have Cesium work out of the box on it and all other popular solutions. Once we're transpiling out of necessity, I expect template strings may be the best solution for the multiline problem and if everything is ES6, hopefully the unknownContextCritical issue evaporates as well.

But just to be clear, both of these issues boils down to Webpack not being able to handle what is otherwise valid JavaScript. These are Webpack bugs. I have mixed feelings with making Cesium changes to work around deficiencies in other tools, I'd rather see our users push on the tool authors to fix their projects. (and for users to understand the tools they are using enough to work around any deficiencies).

So we'll definitely address this, but it will happen as part of the larger module effort.

Member

mramato commented Jan 25, 2017

Ultimately we want to make Cesium work out of the box with all of the most popular build solutions, which is why we'll most likely move to ES6 modules sometime this year. I expect there may be some Webpack specific issues, but the goal would be to have Cesium work out of the box on it and all other popular solutions. Once we're transpiling out of necessity, I expect template strings may be the best solution for the multiline problem and if everything is ES6, hopefully the unknownContextCritical issue evaporates as well.

But just to be clear, both of these issues boils down to Webpack not being able to handle what is otherwise valid JavaScript. These are Webpack bugs. I have mixed feelings with making Cesium changes to work around deficiencies in other tools, I'd rather see our users push on the tool authors to fix their projects. (and for users to understand the tools they are using enough to work around any deficiencies).

So we'll definitely address this, but it will happen as part of the larger module effort.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jan 25, 2017

Contributor

Fair points. Thanks for the response!

(Still curious if @TheLarkInn has any thoughts on how Webpack is handling things.)

Contributor

markerikson commented Jan 25, 2017

Fair points. Thanks for the response!

(Still curious if @TheLarkInn has any thoughts on how Webpack is handling things.)

@js2me

This comment has been minimized.

Show comment
Hide comment
@js2me

js2me Feb 22, 2017

@mramato We look forward to this transition :-)

js2me commented Feb 22, 2017

@mramato We look forward to this transition :-)

@TheLarkInn

This comment has been minimized.

Show comment
Hide comment
@TheLarkInn

TheLarkInn Feb 24, 2017

@mramato What specifically are the bugs that you are referring to? I'm happy to try and get them solved so this isn't a problem.

TheLarkInn commented Feb 24, 2017

@mramato What specifically are the bugs that you are referring to? I'm happy to try and get them solved so this isn't a problem.

@mramato

This comment has been minimized.

Show comment
Hide comment
@mramato

mramato Feb 24, 2017

Member

@TheLarkInn These are the two issues that people run into with Cesium: webpack/webpack#1161 and webpack/webpack#2670

While I'm no webpack expert, I believe they are both cases where webpack fails with otherwise valid and ES5 compliant JavaScript. We're happy to hear any recommendations you may have, especially if there's a trivial workaround on our end.

Member

mramato commented Feb 24, 2017

@TheLarkInn These are the two issues that people run into with Cesium: webpack/webpack#1161 and webpack/webpack#2670

While I'm no webpack expert, I believe they are both cases where webpack fails with otherwise valid and ES5 compliant JavaScript. We're happy to hear any recommendations you may have, especially if there's a trivial workaround on our end.

@TJKoury

This comment has been minimized.

Show comment
Hide comment
@TJKoury

TJKoury Jun 22, 2017

Contributor

Check out our webpack-cesium on npm and let me know if that fulfills the requirements.

Contributor

TJKoury commented Jun 22, 2017

Check out our webpack-cesium on npm and let me know if that fulfills the requirements.

@ggetz ggetz referenced this issue Sep 7, 2017

Closed

Cesium and Webpack #5816

3 of 3 tasks complete
@mramato

This comment has been minimized.

Show comment
Hide comment
@mramato

mramato Oct 20, 2017

Member

This should all be addressed by our recommended webpack configuration and example repository.
(Also includes some minor cesium code changes that will be in the next release).
See https://cesiumjs.org/tutorials/cesium-and-webpack/

Member

mramato commented Oct 20, 2017

This should all be addressed by our recommended webpack configuration and example repository.
(Also includes some minor cesium code changes that will be in the next release).
See https://cesiumjs.org/tutorials/cesium-and-webpack/

@mramato mramato closed this Oct 20, 2017

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