feat: define collections using registerPlugin #5196

aldeed · 2019-06-03T20:03:20Z

Epic #4996
Resolves #4268
Impact: minor
Type: feature

Issue

Most MongoDB collections are defined in a single place in core code, breaking the plugin pattern. Some newer collections are defined in plugin startup functions, but this pattern does not give good control over when the collections exist. Ideally they should already be available when all plugin startup functions run.

Solution

Introduces a new collections option for registerPlugin, which allows plugins to define their MongoDB collections and indexes in a standard way, such that they'll exist already when plugin startup functions run.

Breaking changes

None. Custom plugins may still define collections in a startup function, but you are encouraged to move to the new pattern.

Additional changes

The TestApp class was duplicating several blocks of code from ReactionNodeApp class. Now, TestApp wraps and calls through to a ReactionNodeApp instance.
The collection changes caused timing failures in Meteor app tests. I added Reaction.onAppStartupComplete as a better way to stop all tests from running until after all collections are available and all startup code has run.
In order for integration tests to continue working, I had to move all plugins with collections to the newer non-Meteor registerPlugin pattern.
The collection pattern changes added to startup confusion around file-collections startup. To resolve this, I moved around some files plugin code and eliminated some code that was duplicated between non-Meteor and Meteor startup. None of the actual files code logic is changed; it's just called in better places and in a clearer order.

Testing

Smoke test the API and verify that everything continues to work.
Smoke test the Catalyst UI and verify that everything continues to work.
Smoke test the example storefront connected to the API in this branch and verify that everything continues to work.

Signed-off-by: Eric Dobbertin <[email protected]>

plugin register Signed-off-by: Eric Dobbertin <[email protected]>

Signed-off-by: Eric Dobbertin <[email protected]>

for ReactionNodeApp Signed-off-by: Eric Dobbertin <[email protected]>

Signed-off-by: Eric Dobbertin <[email protected]>

to fix duplicate loading when running within Meteor Signed-off-by: Eric Dobbertin <[email protected]>

Signed-off-by: Eric Dobbertin <[email protected]>

aldeed · 2019-06-03T20:08:09Z

Reviewer note: Unfortunately this was the type of change that cascaded into needing several changes that I couldn't pull apart. The diff looks worse than it is. All the register.js files are just copy/paste from the Meteor file into the non-Meteor file, plus add collections option. In many cases the collections were moved out of startup.js, so that means all of those files changed, too. Beyond that, most changes were to get tests passing again.

brent-hoover

I had a couple of questions and notes but nothing blocking.

brent-hoover · 2019-06-04T00:43:23Z

imports/node-app/core/ReactionNodeApp.js

@@ -74,7 +79,32 @@ export default class ReactionNodeApp {

  setMongoDatabase(db) {
    this.db = db;
-    defineCollections(this.db, this.collections);
+
+    for (const pluginName in this.registeredPlugins) {


This could definitely use some comments to break up what's happening

brent-hoover · 2019-06-04T00:45:53Z

imports/node-app/core/ReactionNodeApp.js

+              }
+              this.collections[collectionKey] = this.db.collection(collectionConfig.name);
+              if (Array.isArray(collectionConfig.indexes)) {
+                for (const indexArgs of collectionConfig.indexes) {


Just curious why you chose to use the for over using something like Array.forEach. It seems like generally we prefer the more javascript idomatic Array.forEach?

I don't remember why. I may have initially been awaiting the collectionIndex call?

brent-hoover · 2019-06-04T02:07:09Z

imports/plugins/core/accounts/server/no-meteor/register.js

+        indexes: [
+          // Create indexes. We set specific names for backwards compatibility
+          // with indexes created by the aldeed:schema-index Meteor package.
+          [{ groups: 1 }, { name: "c2_groups" }],


Is there no way to not use these old index names?

If there's a way to change index names, I suppose we could do a migration. But without that, if you leave out the name and they have the old names already in the db, you get an error about trying to create the same index with a different name. There may be some way, but it would take experimentation.

brent-hoover · 2019-06-04T02:37:23Z

imports/plugins/core/discounts/server/no-meteor/register.js

@@ -0,0 +1,28 @@
+import startup from "./startup";


Just an aside that if I was to nominate a plugin to be removed this would def be one of them

brent-hoover · 2019-06-04T02:40:03Z

imports/plugins/core/email/server/no-meteor/register.js

@@ -0,0 +1,62 @@
+import mutations from "./mutations";


I feel like moving these to no-meteor/register is confusing. Does moving it to this directory enable any different functionality or is it just a mnemonic?

It's mnemonic, but the new way of exporting a single register function that accepts app argument is important. It's a stepping stone toward NPM packages. Imagine the next step where everything in no-meteor moves to a separate package. Then the original, outer register.js file will import register from "@reactioncommerce/plugin-email/register"; and pass it the app to connect it.

The step after that is removing the Meteor auto-register stuff and having people explicitly call the plugins in the order they wish, like is done in registerPlugins.js. That ultimately will be the pattern when the API is fully transitioned to pure Node app.

brent-hoover · 2019-06-04T02:40:47Z

imports/plugins/core/email/server/no-meteor/util/getShopLogo.js

@@ -8,6 +6,9 @@ import { Media } from "/imports/plugins/core/files/server";
 * @return {String} Absolute image URL
 */
 export default async function getShopLogo(context, shop) {
+  const { collections } = context;


Could these be combined into one line? I don't see us using collections anywhere else

Yes I do this by habit since we have other places where we need it.

brent-hoover · 2019-06-04T02:42:22Z

imports/plugins/core/files/server/fileCollections.js


-WebApp.connectHandlers.use("/assets/files", downloadManager.connectHandler);
+  WebApp.connectHandlers.use("/assets/uploads", (req, res) => {
+    req.baseUrl = "/assets/uploads"; // tus relies on this being set, which is an Express thing


https://tus.io/

It's what file-collections package uses for uploading.

Signed-off-by: Eric Dobbertin <[email protected]>

manueldelreal

mikemurray

@aldeed I'm seeing this error when I go to http://localhost:3000. Seems like the build started breaking after the merge from develop.

withPrimaryShopId.js:30 Uncaught TypeError: Cannot destructure property `primaryShopId` of 'undefined' or 'null'.
    at withPrimaryShopId.js:30
    at Query.render (modules.js?hash=8da96538218792e3708caf7d13f2ced0f2877951:287545)
    at finishClassComponent (modules.js?hash=8da96538218792e3708caf7d13f2ced0f2877951:193906)
    at updateClassComponent (modules.js?hash=8da96538218792e3708caf7d13f2ced0f2877951:193868)
    at beginWork (modules.js?hash=8da96538218792e3708caf7d13f2ced0f2877951:194537)
    at performUnitOfWork (modules.js?hash=8da96538218792e3708caf7d13f2ced0f2877951:196576)
    at workLoop (modules.js?hash=8da96538218792e3708caf7d13f2ced0f2877951:196615)
    at HTMLUnknownElement.callCallback (modules.js?hash=8da96538218792e3708caf7d13f2ced0f2877951:180812)
    at Object.invokeGuardedCallbackDev (modules.js?hash=8da96538218792e3708caf7d13f2ced0f2877951:180850)
    at invokeGuardedCallback (modules.js?hash=8da96538218792e3708caf7d13f2ced0f2877951:180899)

brent-hoover · 2019-06-07T06:27:30Z

Not to clutter this up since this is already a big one, but having given it some more thought I am thinking we should migrate these indexes to a standard naming convention now.

It could also be a separate PR but I think if we don't do it now I feel like we will never do it.

I am not blocking this PR for that but it is my suggestion.

aldeed · 2019-06-07T15:59:55Z

@zenweasel That makes sense to me. I'll look into it for either this PR or another.

Signed-off-by: Eric Dobbertin <[email protected]>

aldeed · 2019-06-09T21:40:20Z

@mikemurray After another merge from develop and a few random other fixes, I'm not seeing the error you mention.

mikemurray · 2019-06-10T20:33:18Z

@aldeed 👍 I will run it again

mikemurray

@aldeed Still getting a white screen and the error I posted above. Purged, cleaned, downed, restarted, but nothing. Could there be some configuration I'm missing?

In terminal I'm also seeing issues with migrations when sharting up:

reaction_1  | 20:59:56.493Z ERROR Reaction: Cannot destructure property `Accounts` of 'undefined' or 'null'.
reaction_1  |   TypeError: Cannot destructure property `Accounts` of 'undefined' or 'null'.
reaction_1  |       at Promise.asyncApply (imports/plugins/core/accounts/server/no-meteor/util/addRolesToGroups.js:75:3)
reaction_1  |       at /home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/fiber_pool.js:43:40
reaction_1  |    => awaited here:
reaction_1  |       at Function.Promise.await (/home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/promise_server.js:56:12)
reaction_1  |       at Promise.asyncApply (imports/plugins/core/accounts/server/no-meteor/util/addRolesToGroups.js:58:3)
reaction_1  |       at /home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/fiber_pool.js:43:40
reaction_1  |    => awaited here:
reaction_1  |       at Function.Promise.await (/home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/promise_server.js:56:12)
reaction_1  |       at Promise.asyncApply (imports/plugins/core/accounts/server/no-meteor/startup.js:171:5)
reaction_1  |       at /home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/fiber_pool.js:43:40
reaction_1  |    => awaited here:
reaction_1  |       at Function.Promise.await (/home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/promise_server.js:56:12)
reaction_1  |       at Promise.asyncApply (imports/node-app/core/ReactionNodeApp.js:187:9)
reaction_1  |       at /home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/fiber_pool.js:43:40
reaction_1  |    => awaited here:
reaction_1  |       at Function.Promise.await (/home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/promise_server.js:56:12)
reaction_1  |       at Promise.asyncApply (imports/plugins/core/core/server/startup/startNodeApp.js:68:5)
reaction_1  |       at /home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/fiber_pool.js:43:40

reaction_1  | 20:59:56.629Z ERROR Reaction:
reaction_1  |   Error in migration 24, publishProductsToCatalog TypeError: Cannot read property 'findOne' of undefined
reaction_1  |       at Promise.asyncApply (imports/plugins/core/catalog/server/no-meteor/utils/publishProductToCatalogById.js:13:25)
reaction_1  |       at /home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/fiber_pool.js:43:40
reaction_1  |    => awaited here:
reaction_1  |       at Function.Promise.await (/home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/promise_server.js:56:12)
reaction_1  |       at Promise.asyncApply (imports/plugins/core/catalog/server/no-meteor/utils/publishProductsToCatalog.js:13:16)
reaction_1  |       at /home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/fiber_pool.js:43:40
reaction_1  |    => awaited here:
reaction_1  |       at Function.Promise.await (/home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/promise_server.js:56:12)
reaction_1  |       at Object.up (imports/plugins/core/versions/server/migrations/24_publish_all_existing_visible_products.js:19:17)
reaction_1  |       at migrate (packages/percolate_migrations.js:233:25)
reaction_1  |       at Object.Migrations._migrateTo (packages/percolate_migrations.js:253:7)
reaction_1  |       at Object.Migrations.migrateTo (packages/percolate_migrations.js:167:10)
reaction_1  |       at appEvents.on (imports/plugins/core/versions/server/startup.js:44:9)
reaction_1  |       at Promise.asyncApply (imports/node-app/core/util/appEvents.js:24:11)
reaction_1  |       at /home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/fiber_pool.js:43:40

reaction_1  | 20:59:56.637Z ERROR Reaction:
reaction_1  |   Error while migrating TypeError: Cannot read property 'find' of undefined
reaction_1  |       at Promise.asyncApply (imports/plugins/core/versions/server/no-meteor/util/findAndConvertInBatches.js:21:18)
reaction_1  |       at /home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/fiber_pool.js:43:40
reaction_1  |    => awaited here:
reaction_1  |       at Function.Promise.await (/home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/promise_server.js:56:12)
reaction_1  |       at Object.up (imports/plugins/core/versions/server/migrations/28_add_hash_to_products.js:11:5)
reaction_1  |       at migrate (packages/percolate_migrations.js:233:25)
reaction_1  |       at Object.Migrations._migrateTo (packages/percolate_migrations.js:253:7)
reaction_1  |       at Object.Migrations.migrateTo (packages/percolate_migrations.js:167:10)
reaction_1  |       at appEvents.on (imports/plugins/core/versions/server/startup.js:44:9)
reaction_1  |       at Promise.asyncApply (imports/node-app/core/util/appEvents.js:24:11)
reaction_1  |       at /home/node/.meteor/packages/promise/.0.11.2.1gwulfb.vjny++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/fiber_pool.js:43:40
reaction_1  | Note: you are using a pure-JavaScript implementation of bcrypt.
reaction_1  | While this implementation will work correctly, it is known to be
reaction_1  | approximately three times slower than the native implementation.
reaction_1  | In order to use the native implementation instead, run
reaction_1  |
reaction_1  |   meteor npm install --save bcrypt

mikemurray

Got it to work, and it looks good.

aldeed added 28 commits June 2, 2019 10:51

feat: define collections with registerPlugin

b5da013

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: use standard pattern for files

f7fc8ce

plugin register Signed-off-by: Eric Dobbertin <[email protected]>

test: avoid dup keys in fake data

a5a0b98

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor accounts plugin register

fd347bd

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor cart plugin register

65c3b03

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor catalog plugin register

2cc20d2

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor core plugin register

85f8341

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor discounts plugin register

fcbbb70

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor email plugin register

b522579

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor files plugin register

2b40043

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor i18n plugin register

1ce3782

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor navigation plugin register

f1fadb3

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor orders plugin register

bf9e918

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor product plugin register

a135322

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor tags plugin register

a5f50b3

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor templates plugin register

8974cbf

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor notifications plugin register

bdee007

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor shipping-rates plugin register

b14f147

Signed-off-by: Eric Dobbertin <[email protected]>

fix: SimpleInventory undefined

f9204e0

Signed-off-by: Eric Dobbertin <[email protected]>

fix: handle no primary shop yet

9e87d5d

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor surcharges plugin register

f1772fa

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: no-meteor tax-rates plugin register

3ecede3

Signed-off-by: Eric Dobbertin <[email protected]>

feat: getFunctionsOfType option

6d4f7eb

for ReactionNodeApp Signed-off-by: Eric Dobbertin <[email protected]>

feat: skip server start if no port

fe896e9

Signed-off-by: Eric Dobbertin <[email protected]>

refactor: register more no-meteor plugins

2c2b12f

Signed-off-by: Eric Dobbertin <[email protected]>

test: TestApp refactor

b96f3b7

Signed-off-by: Eric Dobbertin <[email protected]>

fix: rearrange file collection loading

4a3c8ca

to fix duplicate loading when running within Meteor Signed-off-by: Eric Dobbertin <[email protected]>

test: ensure app tests run after app startup

0026c94

Signed-off-by: Eric Dobbertin <[email protected]>

aldeed self-assigned this Jun 3, 2019

aldeed requested a review from mikemurray June 3, 2019 20:03

aldeed requested a review from brent-hoover June 3, 2019 20:06

brent-hoover requested a review from manueldelreal June 4, 2019 02:01

brent-hoover approved these changes Jun 4, 2019

View reviewed changes

aldeed added 2 commits June 4, 2019 07:05

docs: add comments in ReactionNodeApp

c30f225

Signed-off-by: Eric Dobbertin <[email protected]>

Merge branch 'develop' into feat-aldeed-4268-collections-config

4884ad9

aldeed mentioned this pull request Jun 4, 2019

[EPIC] Moving the API off Meteor #4996

Closed

45 tasks

manueldelreal approved these changes Jun 4, 2019

View reviewed changes

mikemurray reviewed Jun 5, 2019

View reviewed changes

aldeed added 5 commits June 9, 2019 13:37

Merge branch 'develop' into feat-aldeed-4268-collections-config

ea9e1c3

test: delay app test until app is ready

f23c5da

Signed-off-by: Eric Dobbertin <[email protected]>

test: lint fix

999495b

Signed-off-by: Eric Dobbertin <[email protected]>

fix: fix migration undefined collection issues

6cb15db

Signed-off-by: Eric Dobbertin <[email protected]>

fix: CSP error in browser on /graphql-alpha

c9fae52

Signed-off-by: Eric Dobbertin <[email protected]>

mikemurray reviewed Jun 10, 2019

View reviewed changes

mikemurray approved these changes Jun 10, 2019

View reviewed changes

mikemurray merged commit 860dec0 into develop Jun 10, 2019

mikemurray deleted the feat-aldeed-4268-collections-config branch June 10, 2019 22:16

jeffcorpuz mentioned this pull request Jul 2, 2019

Release v2.0.0 rc.12 #5259

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: define collections using registerPlugin #5196

feat: define collections using registerPlugin #5196

aldeed commented Jun 3, 2019 •

edited

Loading

aldeed commented Jun 3, 2019 •

edited

Loading

brent-hoover left a comment

brent-hoover Jun 4, 2019

brent-hoover Jun 4, 2019

aldeed Jun 4, 2019

brent-hoover Jun 4, 2019

aldeed Jun 4, 2019

brent-hoover Jun 4, 2019

brent-hoover Jun 4, 2019

aldeed Jun 4, 2019

brent-hoover Jun 4, 2019

aldeed Jun 4, 2019

brent-hoover Jun 4, 2019

aldeed Jun 4, 2019

manueldelreal left a comment

mikemurray left a comment

brent-hoover commented Jun 7, 2019

aldeed commented Jun 7, 2019

aldeed commented Jun 9, 2019

mikemurray commented Jun 10, 2019

mikemurray left a comment

mikemurray left a comment •

edited

Loading

feat: define collections using registerPlugin #5196

feat: define collections using registerPlugin #5196

Conversation

aldeed commented Jun 3, 2019 • edited Loading

Issue

Solution

Breaking changes

Additional changes

Testing

aldeed commented Jun 3, 2019 • edited Loading

brent-hoover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manueldelreal left a comment

Choose a reason for hiding this comment

mikemurray left a comment

Choose a reason for hiding this comment

brent-hoover commented Jun 7, 2019

aldeed commented Jun 7, 2019

aldeed commented Jun 9, 2019

mikemurray commented Jun 10, 2019

mikemurray left a comment

Choose a reason for hiding this comment

mikemurray left a comment • edited Loading

Choose a reason for hiding this comment

aldeed commented Jun 3, 2019 •

edited

Loading

aldeed commented Jun 3, 2019 •

edited

Loading

mikemurray left a comment •

edited

Loading