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(typed-cqrs): use symbol field to store type of query #3

Merged
merged 2 commits into from
Jul 11, 2024

Conversation

Papooch
Copy link

@Papooch Papooch commented May 8, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Updates typescript (in order to use symbols), while adding dependency to itself so that the examples type check correctly after npm run build without needing to use link.
  • The type of commands and queries is now stored in a symbol field, which makes it invisible in consumer code (unlike previous implementation based on a random name)
[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

The same as before, only now there's no random property visible on the query.

Issue Number: N/A

Does this PR introduce a breaking change?

[ ] Yes
[x] Maybe, if someone depended on the random property, which I doubt.
[ ] No

Fixes #4

@Papooch Papooch changed the title Build/update feat(typed-cqrs): use symbol field to store type of query May 8, 2024
@ajfranzoia
Copy link

Is this unmaintained?

package.json Outdated
@@ -45,6 +45,7 @@
"url": "https://github.com/nestjs-architects/typed-cqrs/issues"
},
"devDependencies": {
"@nestjs-architects/typed-cqrs": "file:./",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need it?

Copy link
Author

Choose a reason for hiding this comment

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

We don't, I can remove it from the PR if you want, but this removes the need to call npm link in order for the examples to typecheck correctly.

Copy link
Author

Choose a reason for hiding this comment

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

Removed and rebased

package.json Show resolved Hide resolved
@Sikora00
Copy link
Contributor

Cool! amazing contribution @Papooch
@ajfranzoia it hurts...

@Papooch Papooch requested a review from Sikora00 July 10, 2024 19:19
@Sikora00 Sikora00 merged commit 7edfa8a into nestjs-architects:master Jul 11, 2024
0 of 2 checks passed
@Papooch Papooch deleted the build/update branch July 11, 2024 20:54
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.

Query instance contains an unwanted property with confusing name.
3 participants