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

Remove aldeed:schema-index Meteor package, use collectionIndex #5090

Merged
merged 18 commits into from
Apr 3, 2019

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Apr 2, 2019

Resolves #5080
Impact: minor
Type: feature

Changes

The aldeed:schema-index Meteor package takes SimpleSchema options like index: 1 and unique: true and calls MongoDB createIndex based on them. In an effort to reduce Meteor dependencies, all usage of this and the aldeed:schema-index dependency itself has now been removed.

Indexes are now set up by the plugins that care about them. They call collectionIndex in a startup function to do this. This may not be the final way this is done, but this PR is one step out of many to get to where we are going.

The goal was not to make any decisions about the appropriateness of any indexes in this PR. I did notice some that seem to be outdated, but I did not remove them at this time. This is strictly an internal refactor.

There is one way in which this PR is more than a refactor. There were several fields in the Catalog schema that were marked to be indexed, but because that schema was not attached to the Catalog collection in Meteor, the indexes were not actually created. I did copy the useful ones to the new code, so now Catalog collection will have additional indexes created.

Breaking changes

If you have custom plugins that use index or unique options in their schemas, add back the aldeed:schema-index dependency or update the plugins to use collectionIndex function.

Testing

  1. Run the app on develop branch. Verify that your local database has indexes. Save off the list of indexes. For example:
db.getCollectionNames().forEach(function(collection) {
   indexes = db[collection].getIndexes();
   print("Indexes for " + collection + ":");
   printjson(indexes);
});
  1. Drop all collections.
  2. Run the app on this PR branch. Verify that your local database has indexes. Compare the list with the one you saved off. It should have all of the same indexes as before, and perhaps a few additional. (As stated above, some indexes may not make sense anymore, but we will evaluate this in a future PR.)

aldeed added 18 commits April 1, 2019 10:02
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
It is no longer used by any core or included plugins.
If used by custom plugins, add back the dependency
or update the plugins to use `collectionIndex` function.

Signed-off-by: Eric Dobbertin <eric@dairystatedesigns.com>
@aldeed aldeed self-assigned this Apr 2, 2019
@aldeed aldeed mentioned this pull request Apr 2, 2019
@aldeed aldeed requested a review from kieckhafer April 2, 2019 23:19
Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

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

Looks good, indexes are recreated (and some extras).

@@ -16,7 +16,7 @@ import { Metafield } from "./metafield";
* @property {String[]} relatedTagIds optional
* @property {Boolean} isDeleted default value: `false`
* @property {Boolean} isTopLevel required
* @property {Boolean} isVisible defalut value: `true`
Copy link
Member

Choose a reason for hiding this comment

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

nice catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code Spell Checker extension caught it, but I'll take credit. 😄

@@ -29,7 +29,6 @@ export default function defineCollections(db, collections) {
ProductSearch: db.collection("ProductSearch"),
roles: db.collection("roles"),
SellerShops: db.collection("SellerShops"),
Shipping: db.collection("Shipping"),
Copy link
Member

Choose a reason for hiding this comment

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

Assuming this is just because we are creating Shipping inside it's own package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep

@@ -91,7 +82,7 @@ export const Discounts = new SimpleSchema({
},
"conditions.order.min": {
type: Number,
label: "Mininum",
label: "Minimum",
Copy link
Member

Choose a reason for hiding this comment

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

👍

@kieckhafer kieckhafer merged commit 47b7273 into develop Apr 3, 2019
@kieckhafer kieckhafer deleted the refactor-aldeed-5080-collection-indexes branch April 3, 2019 05:55
@jeffcorpuz jeffcorpuz mentioned this pull request Jul 2, 2019
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.

2 participants