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

feat(datasource-customizer): sort enum values in typings file #892

Merged
merged 8 commits into from
Nov 30, 2023

Conversation

xiniria
Copy link
Contributor

@xiniria xiniria commented Nov 29, 2023

Definition of Done

Initially I had implemented sorting for enum values but also plain, nested and flat field names; when rebasing on main I realized someone else had done it in the meantine. Thus there are only additional tests for these, but enum values are still now sorted. This prevents the kind of problem I've had on my project when generating the typings file:

image

General

  • Write an explicit title for the Pull Request, following Conventional Commits specification
  • Test manually the implemented changes
  • Validate the code quality (indentation, syntax, style, simplicity, readability)

Security

  • Consider the security impact of the changes made

Copy link
Contributor

@ghusse ghusse left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution. We'll definitively integrate your changes, as they allow having a more stable output with the typings file.

I have 2 minor concerns regarding the introduced function. If you can do the modifications, I'll merge your PR.

packages/datasource-customizer/src/typing-generator.ts Outdated Show resolved Hide resolved
@xiniria
Copy link
Contributor Author

xiniria commented Nov 30, 2023

@ghusse @Thenkei The "Send Coverage" CI step seems to fail every time, is it something you had already seen in the repo?

@ghusse
Copy link
Contributor

ghusse commented Nov 30, 2023

@ghusse @Thenkei The "Send Coverage" CI step seems to fail every time, is it something you had already seen in the repo?

Yes, it's working for us, when the CI is run on the original repository.

Comment on lines 215 to 216
?.sort((v1, v2) => v1.localeCompare(v2))
.map(v => `'${v.replace(/'/g, "\\'")}'`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
?.sort((v1, v2) => v1.localeCompare(v2))
.map(v => `'${v.replace(/'/g, "\\'")}'`)
?.map(v => `'${v.replace(/'/g, "\\'")}'`)
.sort((v1, v2) => v1.localeCompare(v2))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Thenkei why do you prefer it this way? Normally it should not make any difference on the sorting

@@ -157,7 +164,7 @@ export default class TypingGenerator {

if (depth < maxDepth) {
queue.push(
...Object.entries(collection.schema.fields)
...TypingGenerator.sortedEntries(collection.schema.fields)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not necessary here to have sorted results

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually removing it breaks the flat fields test, so I think it is necessary in order to have ordered flat fields

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see.

I don't know if it's better to do it here, or simply at the end of the function with result.slice().sort(). This would make it more explicit that we want to return sorted results.

I think that your solution works too, so I'm fine with it. I would just move the call to sort at the beginning of the loop to avoid doing it twice on the same elements:

    while (queue.length > 0 && result.length < this.options.maxFieldsCount) {
      const { collection, depth, prefix, traversed } = queue.shift();
      const sortedEntries = TypingGenerator.sortedEntries(collection.schema.fields);

       if (prefix) {
        result.push(
          ...sortedEntries
            .filter(/**/)
           .map(/**/)
        )
      }

      if (depth < maxDepth) {
        queue.push(
          ...sortedEntries
          .filter(/**/)
          .map(/**/)
          .filter(/**/)
      }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ghusse done!

@ghusse
Copy link
Contributor

ghusse commented Nov 30, 2023

Thanks for your contribution!

@ghusse ghusse merged commit 564974d into ForestAdmin:main Nov 30, 2023
17 of 18 checks passed
forest-bot added a commit that referenced this pull request Nov 30, 2023
## [1.6.66](https://github.com/ForestAdmin/agent-nodejs/compare/example@1.6.65...example@1.6.66) (2023-11-30)

### Features

* **datasource-customizer:** sort enum values in typings file ([#892](#892)) ([564974d](564974d))
@forest-bot
Copy link
Member

🎉 This PR is included in version 1.0.50 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

forest-bot added a commit that referenced this pull request Nov 30, 2023
@forest-bot
Copy link
Member

🎉 This PR is included in version 1.6.66 🎉

The release is available on example@1.6.66

Your semantic-release bot 📦🚀

forest-bot added a commit that referenced this pull request Nov 30, 2023
forest-bot added a commit that referenced this pull request Nov 30, 2023
@forest-bot
Copy link
Member

🎉 This PR is included in version 1.36.6 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.38.0 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.0.85 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.3.49 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.0.61 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@forest-bot
Copy link
Member

🎉 This PR is included in version 1.0.74 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

forest-bot added a commit that referenced this pull request Dec 12, 2023
## [1.5.31](https://github.com/ForestAdmin/agent-nodejs/compare/@forestadmin/datasource-mongoose@1.5.30...@forestadmin/datasource-mongoose@1.5.31) (2023-12-12)

### Bug Fixes

* **datasource-sql:** don't return function names when default values are not constants ([#872](#872)) ([db5dba9](db5dba9))
* do not generate typing aliases with dashes ([#883](#883)) ([f971b68](f971b68))
* field formValue is sometimes not correctly provided in execute context ([#894](#894)) ([a24aab1](a24aab1))
* **renameAndRemoveField:** allow to rename or remove a relation by improving the TS typing ([#865](#865)) ([1a6a4b4](1a6a4b4))
* **restart:** the agent should only restart when at least one customisation is installed ([#893](#893)) ([d5e3c15](d5e3c15))
* **schema:** throw an error when enum values are corrupted in the schema ([#877](#877)) ([d4488c9](d4488c9))
* **security:** patch axios dependency vulnerabilities ([#884](#884)) ([a693ace](a693ace))
* send details to frontend on unexpected sequelize errors ([#868](#868)) ([f6cb9a5](f6cb9a5))
* skip mssql tables with dots in their names instead of crashing the agent ([#870](#870)) ([97aea61](97aea61))
* **smart-field:** log error on missing parameter dependencies and avoid crash ([#873](#873)) ([e7f80e2](e7f80e2))
* **typing:** avoid ordering issues that causes issue with typing ([#890](#890)) ([1c9628c](1c9628c))
* user receives 403 on newly created segment query ([#889](#889)) ([b2eb86a](b2eb86a))

### Features

* **datasource-customizer:** add helper to fetch values from selected record in single actions ([#891](#891)) ([24d3e50](24d3e50))
* **datasource-customizer:** sort enum values in typings file ([#892](#892)) ([564974d](564974d))
* **forestadmin-client:** add schema hash to startup logs ([#867](#867)) ([d13a671](d13a671))
forest-bot added a commit that referenced this pull request Dec 12, 2023
## [1.5.26](https://github.com/ForestAdmin/agent-nodejs/compare/@forestadmin/datasource-sequelize@1.5.25...@forestadmin/datasource-sequelize@1.5.26) (2023-12-12)

### Bug Fixes

* **datasource-sql:** don't return function names when default values are not constants ([#872](#872)) ([db5dba9](db5dba9))
* do not generate typing aliases with dashes ([#883](#883)) ([f971b68](f971b68))
* field formValue is sometimes not correctly provided in execute context ([#894](#894)) ([a24aab1](a24aab1))
* **restart:** the agent should only restart when at least one customisation is installed ([#893](#893)) ([d5e3c15](d5e3c15))
* **schema:** throw an error when enum values are corrupted in the schema ([#877](#877)) ([d4488c9](d4488c9))
* **security:** patch axios dependency vulnerabilities ([#884](#884)) ([a693ace](a693ace))
* skip mssql tables with dots in their names instead of crashing the agent ([#870](#870)) ([97aea61](97aea61))
* **smart-field:** log error on missing parameter dependencies and avoid crash ([#873](#873)) ([e7f80e2](e7f80e2))
* **typing:** avoid ordering issues that causes issue with typing ([#890](#890)) ([1c9628c](1c9628c))
* user receives 403 on newly created segment query ([#889](#889)) ([b2eb86a](b2eb86a))

### Features

* **datasource-customizer:** add helper to fetch values from selected record in single actions ([#891](#891)) ([24d3e50](24d3e50))
* **datasource-customizer:** sort enum values in typings file ([#892](#892)) ([564974d](564974d))
forest-bot added a commit that referenced this pull request Dec 12, 2023
## [1.7.43](https://github.com/ForestAdmin/agent-nodejs/compare/@forestadmin/datasource-sql@1.7.42...@forestadmin/datasource-sql@1.7.43) (2023-12-12)

### Bug Fixes

* field formValue is sometimes not correctly provided in execute context ([#894](#894)) ([a24aab1](a24aab1))
* **restart:** the agent should only restart when at least one customisation is installed ([#893](#893)) ([d5e3c15](d5e3c15))
* **typing:** avoid ordering issues that causes issue with typing ([#890](#890)) ([1c9628c](1c9628c))

### Features

* **datasource-customizer:** add helper to fetch values from selected record in single actions ([#891](#891)) ([24d3e50](24d3e50))
* **datasource-customizer:** sort enum values in typings file ([#892](#892)) ([564974d](564974d))
forest-bot added a commit that referenced this pull request Dec 12, 2023
## [1.29.1](https://github.com/ForestAdmin/agent-nodejs/compare/@forestadmin/datasource-toolkit@1.29.0...@forestadmin/datasource-toolkit@1.29.1) (2023-12-12)

### Bug Fixes

* **agent:** add body parser options to allow increase body limits ([#854](#854)) ([a4de720](a4de720))
* **authentication:** don't crash if an error occurs when initializing the authentication client ([#862](#862)) ([265b1b4](265b1b4))
* **authentication:** return errors details during authentication instead of generic 500s ([#857](#857)) ([3ec14e6](3ec14e6))
* **charts:** remove option to set zeros instead of null values & do it by default ([#863](#863)) ([5f88663](5f88663))
* **datasource-mongoose:** don't return records for null values of flattened fields when using asModel on object fields ([#853](#853)) ([d4b3f0c](d4b3f0c))
* **datasource-mongoose:** error on a nested field when requesting a child property on a missing value ([#860](#860)) ([6a04be7](6a04be7))
* **datasource-sql:** don't return function names when default values are not constants ([#872](#872)) ([db5dba9](db5dba9))
* do not generate typing aliases with dashes ([#883](#883)) ([f971b68](f971b68))
* field formValue is sometimes not correctly provided in execute context ([#894](#894)) ([a24aab1](a24aab1))
* **renameAndRemoveField:** allow to rename or remove a relation by improving the TS typing ([#865](#865)) ([1a6a4b4](1a6a4b4))
* **restart:** the agent should only restart when at least one customisation is installed ([#893](#893)) ([d5e3c15](d5e3c15))
* **schema:** throw an error when enum values are corrupted in the schema ([#877](#877)) ([d4488c9](d4488c9))
* **security:** patch @babel/traverse dependency vulnerabilities ([#855](#855)) ([505b7fa](505b7fa))
* **security:** patch axios dependency vulnerabilities ([#884](#884)) ([a693ace](a693ace))
* send details to frontend on unexpected sequelize errors ([#868](#868)) ([f6cb9a5](f6cb9a5))
* skip mssql tables with dots in their names instead of crashing the agent ([#870](#870)) ([97aea61](97aea61))
* **smart-field:** log error on missing parameter dependencies and avoid crash ([#873](#873)) ([e7f80e2](e7f80e2))
* support mangoose decimal128 type ([#864](#864)) ([5bbed39](5bbed39))
* **time-based-chart:** don't crash when there is no value and format the data to display "no data" on the front ([#852](#852)) ([3f033eb](3f033eb))
* **typing:** avoid ordering issues that causes issue with typing ([#890](#890)) ([1c9628c](1c9628c))
* user receives 403 on newly created segment query ([#889](#889)) ([b2eb86a](b2eb86a))

### Features

* **charts:** timebasedCharts: add option to display missing points as zeros ([#861](#861)) ([b314d3a](b314d3a))
* **datasource-customizer:** add helper to fetch values from selected record in single actions ([#891](#891)) ([24d3e50](24d3e50))
* **datasource-customizer:** sort enum values in typings file ([#892](#892)) ([564974d](564974d))
* **forestadmin-client:** add schema hash to startup logs ([#867](#867)) ([d13a671](d13a671))
forest-bot added a commit that referenced this pull request Dec 12, 2023
## [1.25.2](https://github.com/ForestAdmin/agent-nodejs/compare/@forestadmin/forestadmin-client@1.25.1...@forestadmin/forestadmin-client@1.25.2) (2023-12-12)

### Bug Fixes

* **datasource-sql:** don't return function names when default values are not constants ([#872](#872)) ([db5dba9](db5dba9))
* field formValue is sometimes not correctly provided in execute context ([#894](#894)) ([a24aab1](a24aab1))
* **restart:** the agent should only restart when at least one customisation is installed ([#893](#893)) ([d5e3c15](d5e3c15))
* **typing:** avoid ordering issues that causes issue with typing ([#890](#890)) ([1c9628c](1c9628c))

### Features

* **datasource-customizer:** add helper to fetch values from selected record in single actions ([#891](#891)) ([24d3e50](24d3e50))
* **datasource-customizer:** sort enum values in typings file ([#892](#892)) ([564974d](564974d))
@forest-bot
Copy link
Member

🎉 This PR is included in version 1.5.31 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

@xiniria xiniria deleted the sort-typing-fields branch December 12, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants