Skip to content

Conversation

@yusinto
Copy link
Contributor

@yusinto yusinto commented Jul 19, 2023

This PR is the result of running prettier v3 #208 on all files in the codebase.

@yusinto yusinto changed the base branch from main to yus/prettier-changes July 19, 2023 22:55
* dependencies
* @launchdarkly/akamai-edgeworker-sdk-common bumped from ^0.1.0 to ^0.1.1
* @launchdarkly/js-server-sdk-common bumped from ^1.0.2 to ^1.0.3
- The following workspace dependencies were updated
Copy link
Member

Choose a reason for hiding this comment

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

Prettier MUST not change this file. We should make exclusions for it. Otherwise the file will always be in flux.

I would be fine removing markdowns entirely from its purview. I don't really see any benefit in it reformatting the readme, but this file just needs to not be changed by it.

Copy link
Member

Choose a reason for hiding this comment

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

(This applies to all the changelogs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I've excluded all changelogs.

Repo: https://github.com/akamai/edgeworkers-examples/tree/master/edgekv/lib
*/

import { TextDecoderStream } from 'text-encode-transform';
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with import sorting in groups. I am concerned by sorting inputs into a single group. It is probably fine, but it concerns me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is an anomaly. Firstly it's in .js and not .ts so it's' type and lint skipped very worryingly. If this file was linted, it will not have passed linting because the edgekv_tokens.js import does not exist. I think this entire file was copied from Akamai and its contents are purely from an external source.

Coming back to this pr though, grouping works and it looks like this:

import { EdgeProvider } from '@launchdarkly/akamai-edgeworker-sdk-common';

import { EdgeKV } from './edgekv';

type EdgeKVProviderParams = {
  namespace: string;
  group: string;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm excluding this file from prettier because it seems this is a third party external file which we duplicated and so may want to maintain in its original form.

'use client';
// The "use client" is necessary here because the LaunchDarkly React SDK returns a client-side component

// The "use client" is necessary here because the LaunchDarkly React SDK returns a client-side component
Copy link
Member

Choose a reason for hiding this comment

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

This detached the comment from what it is discussing. Does 'use client'; need to be at the top, or could this comment be before to keep them associated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spent time looking into this. use client is a non-standard directive specific only to nextjs ssr apps. Prettier does not understand this so it does its best.

I have modified the location of the comments to be above the exported function, with updated description to refer and explain this non-standard directive.

@yusinto yusinto force-pushed the yus/prettier-changes-ran branch from 1d8fd36 to 78c73b0 Compare July 28, 2023 21:35
@yusinto yusinto closed this Jul 28, 2023
@yusinto yusinto force-pushed the yus/prettier-changes-ran branch from 78c73b0 to 906c143 Compare July 28, 2023 21:41
@yusinto yusinto reopened this Jul 28, 2023
@yusinto yusinto requested a review from kinyoklion July 28, 2023 22:33
@@ -1 +1,13 @@
{"packages/shared/common":"1.0.2","packages/shared/sdk-server":"1.0.6","packages/sdk/server-node":"8.1.0","packages/sdk/cloudflare":"2.0.7","packages/shared/sdk-server-edge":"1.0.6","packages/sdk/vercel":"1.0.1","packages/sdk/akamai-base":"1.0.0","packages/sdk/akamai-edgekv":"1.0.0","packages/shared/akamai-edgeworker-sdk":"0.2.5","packages/store/node-server-sdk-dynamodb":"5.0.4","packages/store/node-server-sdk-redis":"3.0.4"}
{
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if this will cause problems. I guess we will find out when we release it.

@yusinto yusinto merged commit 1e46326 into yus/prettier-changes Jul 28, 2023
@yusinto yusinto deleted the yus/prettier-changes-ran branch July 28, 2023 22:59
yusinto added a commit that referenced this pull request Jul 28, 2023
This PR only contains config changes. The post-prettier-ran after
effects are in #209 where all the CI checks are green.

* Updated prettier to v3.0
* Added `prettier-plugin-sort-imports` to sort imports

The main files to review are the root `.prettierrc` and root
`package.json`. The other package.json updates are just mirrors of the
root.

---------

Co-authored-by: LaunchDarklyReleaseBot <launchdarklyreleasebot@launchdarkly.com>
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.

3 participants