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.

Sign up for GitHub

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

Jump to bottom

Configurable resource - And more. #11

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Configurable resource - And more. #11

wants to merge 9 commits into from
+7,359 −1,393

Conversation

Bnaya
Copy link

@Bnaya Bnaya commented Aug 8, 2019 •
edited
Loading

Impalements #9
fixes #9
The readme changes will explain the best.

  • Adds ability to configure joins/selects per resource/reference.
  • Fetch full and fresh copy of the record after create and update (More correct behaviour for RA expectations)
  • use prettier, typescript and other infra changes.
  • Some tests

@Bnaya Bnaya mentioned this pull request Aug 8, 2019
Closed
@Bnaya
Copy link
Author

Bnaya commented Aug 9, 2019

A published version of it:
https://www.npmjs.com/package/bnaya_fork_ra-data-nest-crud

grigoreme
grigoreme suggested changes Aug 23, 2019
View reviewed changes
Copy link
Contributor

@grigoreme grigoreme left a comment

Choose a reason for hiding this comment

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

Please remove all the ts-ignore comments, that's way too hacky.
Please argue why do you need @internal at almost every method.
Avoid using : any at all.
Never use import inside a function/property.
Would be much better to have some more smaller files instead of a huge one.

.vscode/extensions.json Outdated Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/nonePublicStuff.ts Outdated Show resolved Hide resolved
src/nonePublicStuff.ts Outdated Show resolved Hide resolved
src/nonePublicStuff.ts Outdated Show resolved Hide resolved
src/nonePublicStuff.ts Outdated Show resolved Hide resolved
src/testsHelpers.ts Outdated Show resolved Hide resolved
@Bnaya
Copy link
Author

Bnaya commented Aug 23, 2019

This kind of code is impossible to strictly type.
The objects we are dealing with are very dynamic.
Adding some local types in some spots is more personal flavour than technical fact.

@Bnaya
Copy link
Author

Bnaya commented Aug 23, 2019

I really appreciate your time reviewing this PR,
But the review is missing the whole point of the PR, and not really valuable.
Giving generic comments about code style, that are also arguable,
Is not really helpful.

There are very good reasons here to use ts-ignore and any
And if you have any style rules to follow, please, lets add eslint config with them.

I expect questions on the implementation details and the api,
And suggestions how to make it better.

Bnaya
Bnaya commented Aug 30, 2019
View reviewed changes
package.json Outdated
@@ -15,7 +15,6 @@
"test": "jest",
"tsc": "tsc",
"eslint": "eslint",
"build-ts-declerations": "tsc -p tsconfig.build.json --emitDeclarationOnly --noEmit false --declaration --isolatedModules false",
Copy link
Author

Choose a reason for hiding this comment

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

Where did it go?

@rayman1104 rayman1104 mentioned this pull request May 21, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grigoreme grigoreme grigoreme approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Discussion/RFC] Configurable custom resources variants
2 participants
@Bnaya @grigoreme