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

Bugs in upsert hooks (and fixed code formatting) #258

Merged
merged 5 commits into from
Nov 22, 2019
Merged

Conversation

SimonSimCity
Copy link
Member

Among the stuff with spaces and commas were some missing imports and some other small things ...

Copy link
Member Author

@SimonSimCity SimonSimCity left a 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.

@@ -25,35 +24,6 @@ export const CollectionHooks = {
}
}

CollectionHooks.getUserId = function getUserId () {
Copy link
Member Author

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


// 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 () {
Copy link
Member Author

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
Copy link
Member Author

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.

@SimonSimCity
Copy link
Member Author

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?

@SimonSimCity SimonSimCity changed the title Fixed code formatting according to eslint configuration Bugs in upsert (and fixed code formatting) Nov 14, 2019
@SimonSimCity SimonSimCity changed the title Bugs in upsert (and fixed code formatting) Bugs in upsert hooks (and fixed code formatting) Nov 14, 2019
@SimonSimCity
Copy link
Member Author

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.

@StorytellerCZ
Copy link
Member

I'm fine to release as 1.0.1.
You are correct @SimonSimCity, we need to add eslint to CI. After this gets merge, let's make another PR for that.

@sebakerckhof
Copy link
Contributor

sebakerckhof commented Nov 20, 2019

LGTM, let's publish as 1.0.1.
(And sorry for the sloppiness in the 1.0.0 rewrite)

@SimonSimCity
Copy link
Member Author

Please be clear about who is responsible for publishing. If you want, I can also publish it.

@StorytellerCZ StorytellerCZ merged commit fced7c9 into master Nov 22, 2019
@StorytellerCZ
Copy link
Member

I'm publishing it today after I update changelog and necessities.

@StorytellerCZ StorytellerCZ deleted the eslint branch November 23, 2019 04:56
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.

None yet

3 participants