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

Remove trailing commas in CodeGen #73

Closed
trivikr opened this issue Jan 4, 2020 · 6 comments
Closed

Remove trailing commas in CodeGen #73

trivikr opened this issue Jan 4, 2020 · 6 comments

Comments

@trivikr
Copy link
Contributor

trivikr commented Jan 4, 2020

The repo aws/aws-sdk-js-v3 uses prettier for formatting which doesn't add trailing commas by default. And we plan to continue using default options of prettier
Docs: https://prettier.io/docs/en/options.html#trailing-commas

We're running prettier on precommit hook to overwrite code generated by CodeGen aws/aws-sdk-js-v3#631
And noticed that trailing commas are added by smithy-typescript-codegen in aws/aws-sdk-js-v3@5f9fbfc

It's optional for smithy-typescript to remove addition of trailing commas.
It won't affect aws/aws-sdk-js-v3 as prettier overwrites the code on precommit hook.

@trivikr
Copy link
Contributor Author

trivikr commented Jan 6, 2020

prettier throws following error in zsh while doing CodeGen for multiple packages:

zsh: argument list too long: prettier

I faced this error while generating rest-json clients aws/aws-sdk-js-v3#643, fixed it temporarily by switching to bash

@trivikr
Copy link
Contributor Author

trivikr commented Jan 7, 2020

lint-staged showed the following warning while committing update to aws.json-1.1 clients:

⚠  lint-staged generated an argument string of 237405 characters, and commands might not run correctly on your platform.
It is recommended to use functions as linters and split your command based on the number of staged files. For more info, please visit:
https://github.com/okonet/lint-staged#using-js-functions-to-customize-linter-commands

Screenshot:
Screen Shot 2020-01-07 at 11 39 07 AM

The number of files which need to be prettified at lint-staged can be reduced if trailing comma is removed in codegen

@trivikr trivikr changed the title [Optional] remove trailing commas in CodeGen Remove trailing commas in CodeGen Jan 9, 2020
@trivikr
Copy link
Contributor Author

trivikr commented Jan 9, 2020

fixed it temporarily by switching to bash

The fix is not working anymore in bash, and git commit fails with following error

spawn E2BIG

I faced this issue while updating clients with fix from aws/aws-sdk-js-v3#696

The prettier command also fails in bash:

$ prettier --write client-*/**/*.ts
bash: /Users/trivikr/.nvm/versions/node/v12.14.0/bin/prettier: Argument list too long

@trivikr
Copy link
Contributor Author

trivikr commented Jan 9, 2020

Temporarily fixed the issue by running prettier on each file instead:

$ git diff --name-only --cached | xargs -I '{}' prettier --write '{}'

@trivikr
Copy link
Contributor Author

trivikr commented Jan 9, 2020

Running prettier on each staged file takes lot of time

A faster fix is running prettier per client instead of per file

for f in clients/*
do
  ./node_modules/.bin/prettier --write $f/**/*.{ts,md,json}
done

@trivikr
Copy link
Contributor Author

trivikr commented Jan 9, 2020

temporary commit which added aws.json-1.0 clients without running prettier trivikr/aws-sdk-js-v3@476e88b

It has several changes other than trailing commas, as shown in the screenshot below
Screen Shot 2020-01-09 at 12 32 31 PM

As discussed offline:

  • CodeGen will only be responsible for generating valid TypeScript code
  • Formatting (and in future linting) will be done by JavaScript code in aws-sdk-js-v3

Closing this in favor of aws/aws-sdk-js-v3#698

@trivikr trivikr closed this as completed Jan 9, 2020
trivikr added a commit to trivikr/aws-sdk-js-v3 that referenced this issue Jan 9, 2020
srchase pushed a commit that referenced this issue Mar 23, 2023
* Remove SJCL from browser random source

* Use crypto feature detection rather than generic runtime detection

* Remove packages vendoring portions of the SJCL

* Fix flaky signature tests
srchase pushed a commit that referenced this issue Jun 16, 2023
* Remove SJCL from browser random source

* Use crypto feature detection rather than generic runtime detection

* Remove packages vendoring portions of the SJCL

* Fix flaky signature tests
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

No branches or pull requests

1 participant