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

Build pipeline stabilization #17

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

bohoffi
Copy link
Contributor

@bohoffi bohoffi commented Feb 28, 2024

This PR targets some build pipeline issues:

  • demo application budgets
  • the libs dependencies according to the dependency check Lint rule
  • unified usage of shared Empty type
    • type was renamed

Copy link
Collaborator

@rainerhahnekamp rainerhahnekamp left a comment

Choose a reason for hiding this comment

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

Thanks @bohoffi, I've left some comments.

apps/demo/project.json Outdated Show resolved Hide resolved
libs/ngrx-toolkit/package.json Outdated Show resolved Hide resolved
@@ -1,2 +1,2 @@
// eslint-disable-next-line @typescript-eslint/ban-types
export type Emtpy = {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure about that to be honest. I know you just fixed a typo there, but do we really want to call it empty. I can remember of some eslint rule which says Empty should be Record<string, never>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, eslint itself recommends Record<string, never>:

If you want a type meaning "empty object", you probably want Record<string, never> instead.'

What do you think about renaming the type to EmptyObject to create a better alignment between its name and its usage and use Record<string, never> to apply eslints recommendation?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Absolutely, would you actually create an alias for that or go directly with the {}?

I heard once that TypeScript is faster, when using aliases instead of "individual" types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I prefer a type alias as it ensures a consistent code style.
I've just checked what would happen when we'd use Record<string, never> instead of {} and it would prevent state patching as expected as the Record type wouldn't allow any updates.
Maybe I'm trying to overengineer it right now... when looking at its usage it represents that either do not have any specific expectations to the store or do not add anything to state/signals/methods.

I did some research about the performance and it depends - aliases can bloat up the bundle when massively used and inlined.

libs/ngrx-toolkit/src/lib/with-call-state.ts Outdated Show resolved Hide resolved
"dependencies": {
"tslib": "^2.3.0"
"@ngrx/signals": "^17.0.0",
"@ngrx/store": "^17.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we leave that out for now? @mikezks, I think we need to find a better solution here. It shouldn't be that ngrx/store (and rxjs) come as dependency of the toolkit.

Copy link
Collaborator

@mikezks mikezks Mar 1, 2024

Choose a reason for hiding this comment

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

Fully agree. I would not put store and signals into dependencies or peerDependencies.

I may put parts of the Redux Connector into a secondary entry point later to add a peer dependency on the store, but for now I would remove them again.

Edit: Signals into peer is fine, like currently defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The @nx/dependency-checks rule which brought up those changes offers fixers for missing or unused dependencies - the result would be exactly the same as with this PR.

With Mikes potential plan ahead I'll revert this change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mikezks does that actually work?

https://github.com/testing-library/angular-testing-library/tree/main/projects/testing-library/jest-utils has the same issue with a dependency to Jest. The fixed it by not including it into the package.json (like we have it now).

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