-
-
Notifications
You must be signed in to change notification settings - Fork 92
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
Bugs in upsert hooks (and fixed code formatting) #258
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave a reason for removing the individual code-pieces.
collection-hooks.js
Outdated
@@ -25,35 +24,6 @@ export const CollectionHooks = { | |||
} | |||
} | |||
|
|||
CollectionHooks.getUserId = function getUserId () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is already defined and overwritten by the server.js
and client.js
collection-hooks.js
Outdated
|
||
// Make the above available for packages with hooks that want to determine | ||
// whether they are running inside a publish function or not. | ||
CollectionHooks.isWithinPublish = function isWithinPublish () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was falsely changed. This function should only be defined on the server - and it's already defined in the server.js
file.
@@ -4,11 +4,6 @@ import { CollectionHooks } from './collection-hooks' | |||
const isEmpty = a => !Array.isArray(a) || !a.length | |||
|
|||
CollectionHooks.defineAdvice('upsert', function (userId, _super, instance, aspectGroup, getTransform, args, suppressAspects) { | |||
// args[0] : selector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment only describes what line 15 shows in code.
I'll create a second PR on the basis of this where I'll fix an error that should have been caught if eslint would've been used. @StorytellerCZ is it hard to include the linter in the CI? |
…n the original documents
…lds but the selectors fields.
I just realized that I pushed the three bug reports also in here ... so be it so ... Every of those three bugs has a test-case and I'm using most of these features in production these days. All three were introduced now in 1.0.0 - and I can confirm that there were no other code-changes introduced in 1.0.0 other than these - as far as I've seen. Would be nice to get this merged soon. |
I'm fine to release as 1.0.1. |
LGTM, let's publish as 1.0.1. |
Please be clear about who is responsible for publishing. If you want, I can also publish it. |
I'm publishing it today after I update changelog and necessities. |
Among the stuff with spaces and commas were some missing imports and some other small things ...