Skip to content

chore: major prettier update - #208

Merged
yusinto merged 13 commits into
mainfrom
yus/prettier-changes
Jul 28, 2023
Merged

chore: major prettier update#208
yusinto merged 13 commits into
mainfrom
yus/prettier-changes

Conversation

@yusinto

@yusinto yusinto commented Jul 19, 2023

Copy link
Copy Markdown
Contributor

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.

Comment thread .prettierrc Outdated
Comment on lines +4 to +9
"semi": false,
"plugins": ["@trivago/prettier-plugin-sort-imports"],
"importOrder": ["^@launchdarkly/(.*)$", "^[./]"],
"importOrderCaseInsensitive": true,
"importOrderSeparation": true,
"importOrderSortSpecifiers": true

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I got rid of semicolons in other projects and I don't miss them. Let me know if you think they are necessary, I'm more than happy to negotiate.

Sorting imports make our code nicer and it's already part of gonfalon. We are using the prettier plugin here which is better and more modern, compared to the eslint plugin in gonfalon.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I strongly favor semicolons, but I think it is only personal preference. I don't have any reason to add them, and I don't think there is any reason to remove them either. So, I'm not going to stop you.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback. I have re-instated semicolons.

Comment thread package.json
"contract-tests": "npm run contract-test-service & npm run contract-test-harness",
"prettier": "npx prettier --write \"**/*.{js,ts,tsx,json,yaml,yml,md}\" --loglevel warn"
"prettier": "npx prettier --write \"**/*.{js,ts,tsx,json,yaml,yml,md}\" --log-level warn",
"check": "yarn && yarn prettier && yarn lint && tsc && yarn build"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added check as a quick way to validate everything still works. I included tsc to validate typescript and its json config files.

Comment thread package.json
"prettier": "^3.0.0",
"typedoc": "0.23.26",
"typescript": "^4.6.3"
"typescript": "^5.1.6"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Worth pointing out major typescript update here. I don't think this is a big deal because all the other sub projects are already on v5.0.

Comment thread tsconfig.json
Comment on lines -33 to -38
},
{
"path": "./packages/store/node-server-sdk-redis/tsconfig.ref.json"
},
{
"path": "./packages/store/node-server-sdk-dynamodb/tsconfig.ref.json"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yarn check revealed these don't exist, which is true so I deleted them.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Originally I had things compiling effectively as one large project. Now running tsc just double compiles some parts of it, because the build runs tsc on each thing in order. Because the builds had extra requirements like rollup.

},
"peerDependencies": {
"@aws-sdk/client-dynamodb": "^3.53.0",
"@aws-sdk/client-dynamodb": "^3.370.0",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Callout here I had to update this because typescript was erroring. I suspect upgrading from ts 4 to 5 needs this updating.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We chose 53 for specific reasons, and if it doesn't work, then we should figure that out. If it cannot be resolved, then we should choose another version that isn't likely to inconvenience customers.

The peer dependency shouldn't have much of a bearing on if it works though. Things are compiled with the dev dependency. Currently it is the lowest to help maintain compatibility.

What is typescript error?

@yusinto yusinto Jul 21, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I rolled back these changes to reproduce the error but could not. As a result, I am not going to update the aws packages in this PR. I'll keep them the same as main.

@yusinto
yusinto merged commit 4a14933 into main Jul 28, 2023
@yusinto
yusinto deleted the yus/prettier-changes branch July 28, 2023 23:18
atornsii added a commit that referenced this pull request Jul 22, 2026
…AIC-2858) (#1794)

<!-- CURSOR_AGENT_PR_BODY_BEGIN -->
**Requirements**

- [x] I have added test coverage for new or changed functionality
- [x] I have followed the repository's pull request submission
guidelines
- [x] I have validated my changes against all supported platform
versions

**Related issues**

- [AIC-2858](https://launchdarkly.atlassian.net/browse/AIC-2858) — Make
Changes to Node.js
- Parent: [AIC-2849](https://launchdarkly.atlassian.net/browse/AIC-2849)
— Add modelKey and modelVersion as fields to be read by SDK
- Depends on backend payload work:
[AIC-2876](https://launchdarkly.atlassian.net/browse/AIC-2876)
- Mirrors Go:
[AIC-2850](https://launchdarkly.atlassian.net/browse/AIC-2850) / Python:
[PR #208](launchdarkly/python-server-sdk-ai#208)

**Describe the solution you've provided**

Read `modelKey` and `modelVersion` from the AI Config variation payload
(`variation.model`) and stamp them on all `LDAIConfigTracker` metric
event payloads, alongside existing `modelName`/`providerName` fields.

- Add optional `modelKey`/`modelVersion` to `LDModelConfig`
- Pass the new fields into `LDAIConfigTrackerImpl` from
`LDAIClientImpl._evaluate`
- Include `modelVersion` (always) and `modelKey` (when present) in
`getTrackData()`
- Default `modelVersion` to `1` when absent; exclude both fields from
the resumption token
- Additive/backward compatible — older payloads without the new fields
continue to work

**Describe alternatives you've considered**

None — this mirrors the established pattern from the Go and Python SDK
implementations.

**Additional context**

Tech spec: [Models Primitive
Improvements](https://launchdarkly.atlassian.net/wiki/spaces/PD/pages/4977984384/Models+Primitive+Improvements+-+Tech+Spec)

## Test plan

- [x] `yarn workspace @launchdarkly/server-sdk-ai test` (241 passed)
- [x] `yarn workspace @launchdarkly/server-sdk-ai lint`
- [x] `yarn workspaces foreach -pR --topological-dev --from
'@launchdarkly/server-sdk-ai' run build`
- [ ] Verify against a staging environment once AIC-2876 payload is
available
<!-- CURSOR_AGENT_PR_BODY_END -->

<div><a
href="https://cursor.com/agents/bc-cf6a4de0-cf19-4491-b781-9b617da9a68d"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source
media="(prefers-color-scheme: light)"
srcset="https://cursor.com/assets/images/open-in-web-light.png"><img
alt="Open in Web" width="114" height="28"
src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a>&nbsp;<a
href="https://cursor.com/background-agent?bcId=bc-cf6a4de0-cf19-4491-b781-9b617da9a68d"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source
media="(prefers-color-scheme: light)"
srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img
alt="Open in Cursor" width="131" height="28"
src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a>&nbsp;</div>



[AIC-2858]:
https://launchdarkly.atlassian.net/browse/AIC-2858?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[AIC-2849]:
https://launchdarkly.atlassian.net/browse/AIC-2849?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[AIC-2876]:
https://launchdarkly.atlassian.net/browse/AIC-2876?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
[AIC-2850]:
https://launchdarkly.atlassian.net/browse/AIC-2850?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

---------

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Anthony Torns II <atornsii@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.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.

2 participants