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

Update build-cesium task to account for prettier formatting #600

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Aug 3, 2021

Updates the build-cesium task to account for gltf-pipeline being formatted with prettier now.

@lilleyse lilleyse requested a review from ebogo1 August 3, 2021 01:07
@@ -130,7 +130,6 @@ function amdify(source, subDependencyMapping) {
let requirePath;

source = source.replace(/\r\n/g, "\n");
source = source.replace(/\b(let|const)\b/g, "var");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason to use var anymore especially now that CesiumJS has dropped support for IE11.

Copy link
Contributor

Choose a reason for hiding this comment

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

does this also apply to CesiumJS source code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's for us to decide. The GltfPipeline folder in CesiumJS is very self-contained so it's a good testing ground for things like this.

Copy link
Contributor

@ebogo1 ebogo1 left a comment

Choose a reason for hiding this comment

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

Looks good to me; I confirmed this works with CesiumJS 1.84 and that all specs pass just in case. Some questions -

It uses a different version of prettier than CesiumJS so I couldn't remove it from .prettierignore.

You mentioned this in CesiumGS/cesium#9706; is there a reason why the prettier versions are different?

@@ -130,7 +130,6 @@ function amdify(source, subDependencyMapping) {
let requirePath;

source = source.replace(/\r\n/g, "\n");
source = source.replace(/\b(let|const)\b/g, "var");
Copy link
Contributor

Choose a reason for hiding this comment

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

does this also apply to CesiumJS source code?

@lilleyse
Copy link
Contributor Author

lilleyse commented Aug 3, 2021

You mentioned this in CesiumGS/cesium#9706; is there a reason why the prettier versions are different?

Some of our other repos are also using the latest version of prettier. I thought it would be best to use the latest here.

We're free to update the prettier version in CesiumJS as well, it just means we might need to do the whole pre-prettier and post-prettier tags against depending on the extent of changes.

@ebogo1 ebogo1 merged commit a4ddeea into main Aug 4, 2021
@ebogo1 ebogo1 deleted the update-build-cesium branch August 4, 2021 17:39
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.

2 participants