Use NPM SimpleSchema rather than Meteor #3331

aldeed · 2017-11-20T16:19:41Z

BREAKING CHANGES

This PR updates the aldeed:simple-schema Meteor package dependency to instead depend on the simpl-schema NPM package, which is the newest release of the same library. As part of this change, there are several breaking changes and other gotchas to be aware of.

IMPORTANT! The NPM package does not play nice with the previous Meteor package. After updating to this Reaction release, run the app one time, and then look at the .meteor/versions file. Make sure that aldeed:simple-schema is not listed. If it is there, that is because you depend on another Meteor package that depends on aldeed:simple-schema. You will have to update or remove any such packages (with meteor remove / meteor add) until aldeed:simple-schema disappears from your .meteor/versions file.
Search your app for any import { SimpleSchema } from "meteor/aldeed:simple-schema" lines that you have added in your custom code, and replace them with import SimpleSchema from "simpl-schema"
Be aware that the package name does not have the "e" on "simpl". (There is a different NPM package called simple-schema with the "e", and that is NOT the one you want.)
If you have your own custom schemas, refer to the SimpleSchema changelog to update them for the breaking changes: https://github.com/aldeed/meteor-simple-schema/blob/master/CHANGELOG.md#200
If you use attachSchema in your code, be aware that passing an array to attachSchema will no longer work. You should first merge all the schemas and then pass the combined schema to attachSchema

brent-hoover · 2017-12-01T00:20:22Z

@aldeed Hey Eric, I think I speak for everyone when I say we are super-excited to get this work done and to have you doing it. Just starting to review this code now and my first question is that I am unclear on why some of these changes need to be made to support node-simple-schema. I wonder if you could put in some review comments about why some of these changes were made it would make it easier for me to understand what's going on. (this is one area where GH code review is really lacking)

I wonder if maybe we could schedule a time to talk to me and/or the team about this and help us understand how node-simple-schema differs from meteor-simple-schema. It's a pretty core part of our project and everyone on the team needs to have a really good understanding of how it works.

aldeed · 2017-12-01T01:33:32Z

@zenweasel There might be some changes still in here from my trying to debug issues, which I will remove before finalizing. Still need to skim through it all a final time myself. I also fixed some over-reactive autoruns while trying to figure out a bug caused by the updates, but I'll move those changes into a separate PR since they're technically unrelated.

I'll add some details about what things were updated as I do another read-through of the diff. Possibly tomorrow or this weekend.

aaronjudd

Scanned through the changes. Will review in more detail, but for now, when I went to test immediately had error in lib/api/catalog.js getVariants()

Error: You may not pass an array of schemas to the SimpleSchema constructor or to extend()
    at SimpleSchema.extend (/Users/aaronjudd/Projects/reaction/node_modules/simpl-schema/dist/SimpleSchema.js:503:40)
    at new SimpleSchema (/Users/aaronjudd/Projects/reaction/node_modules/simpl-schema/dist/SimpleSchema.js:127:10)
    at ns.Collection.c2AttachSchema [as attachSchema] (packages/aldeed:collection2-core/collection2.js:35:10)
    at collections.js (lib/collections/collections.js:85:8)
    at fileEvaluate (packages/modules-runtime.js:333:9)
    at require (packages/modules-runtime.js:228:16)
    at index.js (lib/collections/index.js:1:14)
    at fileEvaluate (packages/modules-runtime.js:333:9)
    at require (packages/modules-runtime.js:228:16)
    at catalog.js (lib/api/catalog.js:1:163)

aldeed · 2017-12-11T22:52:33Z

@aaronjudd Sorry I had more changes I hadn't pushed. I pushed them now, and should include fixing the error you saw. There are still other more hidden issues I'm investigating, but I think it's getting close.

aldeed

General changes:

Passing arrays to SimpleSchema is no longer supported, so instead I use clone().extend({})
check and Tracker need to be passed as options to every SimpleSchema constructor
type: [Type] isn't supported for arrays, so instead it's type: Array plus a specific separate key for the array items
autoValues work differently now, only setting the value if the parent object exists. Many places in Reaction were relying on the old behavior of an object being auto-created, so I've retained that behavior where necessary by adding defaultValue: {} on the object key's schema.
Changed Number to SimpleSchema.Integer and Number+decimal to just Number
New SS doesn't work with check package, so these usages in methods have been changed to schema.validate(), which does the same thing. It still satisfies audit-argument-checks due to passing check into the SS options.
invalidKeys renamed to validationErrors
Use bypassCollection2 option on migration operations. This is best practice to avoid schema errors since the schema changes over time

aldeed · 2017-12-30T22:33:18Z

client/modules/core/helpers/apps.js

        // If it's not an array, the filter should match exactly
-        return item[property] === itemFilter[property];
+        return Array.isArray(itemVal) ? itemVal.includes(filterVal) : itemVal === filterVal;


This was checking the schema to see if it should be an array, but it actually makes more sense to just check if the value is an array.

aldeed · 2017-12-30T22:35:40Z

client/modules/core/main.js

-
-    return defaultValue || undefined;
+    const prefs = userPrefs.get();
+    return prefs && prefs[packageName] && prefs[packageName][preference] || defaultValue || undefined;


Not sure why, but I had problems with this while trying to fix SS issues. Before it would react every time anything about the user record changed whereas now reactivity is limited to the prefs changing.

aldeed · 2017-12-30T22:37:43Z

client/modules/core/main.js

-      });
-    }
-    return false;
+    const userId = Meteor.userId();


Just a small change here to remove an unnecessary Meteor.user() call. This matters more on the server than on the client (Meteor.user() would hit the database every time whereas userId would not), but it's still a good habit for client code, too.

aldeed · 2017-12-30T22:38:44Z

client/modules/core/startup.js

+    if (!userId) return;
+    const user = Meteor.users.findOne(userId, { fields: { profile: 1 } });
+    userPrefs.set(user && user.profile && user.profile.preferences || undefined);
+  });


This is used by the rewritten getUserPreferences function.

aldeed · 2017-12-30T22:39:29Z

client/modules/i18n/main.js

@@ -42,23 +43,21 @@ export function getBrowserLanguage() {
 export function getLabelsFor(schema, name) {
  const labels = {};
  // loop through all the rendered form fields and generate i18n keys
-  for (const fieldName of schema._schemaKeys) {
+  Object.keys(schema.mergedSchema()).forEach((fieldName) => {


Gotta do this differently since subschemas aren't automatically merged in the new SS

aldeed · 2017-12-30T23:11:41Z

server/imports/fixtures/orders.js

@@ -130,17 +130,20 @@ export default function () {
    requiresShipping: true,
    shipping: [{
      shopId: getShopId(),
+      address: getAddress({ isShippingDefault: true }),


These missing props were causing test failures. I'm pretty sure they were always improperly missing, but for some reason the old SS package didn't catch the errors I guess.

aldeed · 2017-12-30T23:14:27Z

server/methods/catalog.js

+    _id: {
+      $nin: [productId]
+    }
+  }).count();


This fixed an infinite loop I was seeing, though I don't recall how to reproduce it. It should be obvious reading the code that the $nin should have been there.

aldeed · 2017-12-30T23:15:25Z

server/methods/core/orders.app-test.js

      });
-      return done();


done was called too early with these on the outside

aldeed · 2017-12-30T23:18:40Z

server/methods/core/orders.js

+        result = Meteor.call(`${processor}/payment/capture`, paymentMethod);
+      } catch (e) {
+        error = e;
+      }


The only change here is that the Meteor.call of ${processor}/payment/capture is now synchronous (has no callback). Before this change, I had tests failing because that call would still be executing after the test had already finished and removed the sandbox data. Doing it synchronous seems like it would be better in real world, too, but on the off chance that this was intentionally async, there might be a different solution by rewriting how the tests work.

aldeed · 2017-12-30T23:20:11Z

server/startup/init.js

+  ddpError.error = "validation-error";
+  ddpError.details = error.details;
+  return ddpError;
+});


Not sure if this is the proper file to put this in. Let me know if you want it moved.

aldeed · 2017-12-30T23:25:11Z

@zenweasel @aaronjudd I think this is pretty close. Finally figured out all the pesky bugs and got tests passing (on my machine at least). This was much more difficult than I thought, mainly because all of your crazy schemas illuminated several edge case bugs in SimpleSchema. But the good news is that I feel quite confident in npm SimpleSchema now.

I ran through some screens, but I'm not an expert on the app workflows, so it would be helpful to have some manual testing through common flows and with common plugins.

brent-hoover · 2018-01-01T23:09:45Z

Seeing this error on startup:

23:07:51.982Z  WARN Reaction: Error while importing to Packages: Cannot update 'settings' and 'settings' at the same time

brent-hoover · 2018-01-01T23:36:15Z

See this error when I add a product and set the price:

Match error: Failed Match.OneOf, Match.Maybe or Match.Optional validation [server-error]

brent-hoover · 2018-01-01T23:55:52Z

imports/plugins/core/checkout/client/methods/cart.js

@@ -11,7 +11,7 @@ Meteor.methods({
  // Under consideration for deprecation and migrating other payment Packages
  // to payments-stripe style methods
  "cart/submitPayment": function (paymentMethod) {
-    check(paymentMethod, Reaction.Schemas.PaymentMethod);


I think this is actually a pretty important check here that shouldn't just be switched to Object. If the paymentMethod doesn't have specific values it's going to fail hard upstream and we should catch it here.

@zenweasel It might be possible to use the schema, but the problem I was having with using the schema in some places like this is that the payment method being passed is somewhat different from what ultimately goes into the DB, either because of default values or for some other reasons. This object is validated against the schema already as part of the update call on line 59, so it seemed like checking it here is a bit superfluous anyway.

brent-hoover · 2018-01-02T00:01:18Z

imports/plugins/included/discount-rates/server/methods/methods.app-test.js

@@ -42,7 +41,6 @@ describe("discount rate methods", function () {
      expect(discountInsertSpy).to.have.been.called;

      const discountCount = Discounts.find(discountId).count();
-      Meteor._sleepForMs(500);


I'm getting test failures re-introduced by removing these sleeps.

Is it okay if I try to fix the code instead? Needing to sleep for tests is a tenuous fix and just ignores the fact that failures due to timing usually mean poorly written code.

For sure. I would love to get these really fixed. Just pointing out that the tests are failing intermittently now.

brent-hoover

Just a few style notes

brent-hoover · 2018-01-02T00:05:40Z

imports/plugins/included/shipping-shippo/server/methods/shippoapi.js

@@ -2,7 +2,7 @@
 import Shippo from "shippo";
 import { Meteor } from "meteor/meteor";
 import { ValidatedMethod } from "meteor/mdg:validated-method";
-import { SimpleSchema } from "meteor/aldeed:simple-schema";
+import SimpleSchema from "simpl-schema";


import order

brent-hoover · 2018-01-02T00:15:25Z

lib/collections/schemas/products.js

 import { Meteor } from "meteor/meteor";
 import { Random } from "meteor/random";
-import { SimpleSchema } from "meteor/aldeed:simple-schema";
+import { Tracker } from "meteor/tracker";
+import SimpleSchema from "simpl-schema";


import order

brent-hoover · 2018-01-02T00:33:42Z

server/api/core/core.js

-   *                  that have been attached to the collection or false if
-   *                  the collection or schema could not be found
-   */
-  collectionSchema(collection, selector) {


Can we provide any sort of deprecation path there rather than just removing this in case application developers are using it?

brent-hoover · 2018-01-02T00:36:18Z

server/methods/accounts/accounts.app-test.js

@@ -104,6 +104,7 @@ describe("Account Meteor method ", function () {
      const newAddress = account.profile.addressBook[
        account.profile.addressBook.length - 1];
      delete newAddress._id;
+      delete newAddress.failedValidation;


Why are we deleting this here?

Because it is added to the doc automatically due to a defaultValue. I'm not sure why it wasn't an issue prior to my changes, but I'm pretty sure the defaultValue was always there. If you don't want to delete it, then it could be added to the original address being inserted.

No, I was just trying to understand why we were deleting it

brent-hoover · 2018-01-02T00:42:06Z

server/publications/collections/products.js

 import { check, Match } from "meteor/check";
-import { SimpleSchema } from "meteor/aldeed:simple-schema";
+import SimpleSchema from "simpl-schema";


import order

brent-hoover · 2018-01-02T00:42:32Z

server/startup/init.js

@@ -1,6 +1,18 @@
+import { Meteor } from "meteor/meteor";
+import SimpleSchema from "simpl-schema";


import order

brent-hoover · 2018-01-02T01:06:11Z

I don't seem to be getting any validation error when saving invalid settings in any of the settings panels

(as an example, remove "Company Code" from the Avalara tax method and it should throw an error but does not).

aaronjudd · 2018-01-04T17:20:12Z

This is looking really close, fantastic.

I see id/react/validation issues in Taxes, Accounts, Login Services but verified that these console errors all exist in master as well, so not apparently introduced here. The lack of validation, as reported by Brent I am able to replicate as well in the Avalara panel, and is not reproducible in master. Most other forms do seem to have validation though.

I noticed the sort order of the packages (as seen in the icon-side-bar) are different in master versus this branch.

Fatal error in checkout address validation, reproducible by adding address in checkout...

Exception in delivering result of invoking 'accounts/validateAddress': TypeError: Cannot read property 'validated' of undefined
    at http://localhost:3000/app/app.js?hash=8bb9266b26ebdf2fa9b1a45ac3b8ba21dd556a6c:37967:19
    at MethodInvoker._callback (http://localhost:3000/packages/meteor.js?hash=cbcc712d51de4298c275e8dcf25c66c29914f19a:1167:22)
    at MethodInvoker._maybeInvokeCallback (http://localhost:3000/packages/ddp-client.js?hash=df770fd9a6a02fd730939b97d266ea2b12938e95:3682:12)
    at MethodInvoker.receiveResult (http://localhost:3000/packages/ddp-client.js?hash=df770fd9a6a02fd730939b97d266ea2b12938e95:3702:10)
    at Connection._livedata_result (http://localhost:3000/packages/ddp-client.js?hash=df770fd9a6a02fd730939b97d266ea2b12938e95:4824:9)
    at onMessage (http://localhost:3000/packages/ddp-client.js?hash=df770fd9a6a02fd730939b97d266ea2b12938e95:3528:206)
    at http://localhost:3000/packages/ddp-client.js?hash=df770fd9a6a02fd730939b97d266ea2b12938e95:2908:9
    at Array.forEach (<anonymous>)
    at Function._.each._.forEach (http://localhost:3000/packages/underscore.js?hash=cde485f60699ff9aced3305f70189e39c665183c:149:11)
    at SockJS.self.socket.onmessage (http://localhost:3000/packages/ddp-client.js?hash=df770fd9a6a02fd730939b97d266ea2b12938e95:2907:43)

I'll keep poking, but that one at least is a blocker.

brent-hoover · 2018-01-05T03:07:02Z

@aldeed Don't forget to enter yourself for the Hack-a-bug-a-thon We are giving away a $1000 gift card and other prizes

aldeed · 2018-01-08T01:46:55Z

@zenweasel @aaronjudd So far I have fixed the following:

Added back collectionSchema function with new code and a deprecation warning logged.
Fixed import order
Fixed the settings error on startup
Fixed the error when updating variant price. This was coming not from the initial call to products/updateProductField but from the next call triggered by Products.before.update hook. The line const priceRange = ProductRevision.getProductPriceRange(updateId); was returning undefined. I fixed it by updating getProductPriceRange to return range objects if it returns early (in this case, if product.price is undefined). Let me know if that looks okay. I'm not sure if price being undefined is a symptom of a larger issue.

Still need to fix:

Lack of validation in some settings forms
"Fatal error in checkout address validation, reproducible by adding address in checkout..."
Random test failures / removing test sleeps

brent-hoover · 2018-01-30T23:03:24Z

Tested and this last issue seems to be fixed.

brent-hoover

All issues resolved. 👍

DenisBogatirov · 2018-02-05T16:36:24Z

When this will be merged to master?
I have an issue with autoValue, and it should be fixed by updating to npm package of dimple-schema
"createdAt": { "$date": "2014-06-01T12:17:13.949-0700" },
I've removed this part from the Products.json fixture, and createdAt field is missing on products at all.
createdAt: { type: Date, autoValue() { if (this.isInsert) { return new Date(); } else if (this.isUpsert) { return { $setOnInsert: new Date() }; } } }, So this code isn't working at all.

aldeed · 2018-02-05T18:48:49Z

Merge in 1.8.0 and fixed conflicts

DenisBogatirov · 2018-03-02T10:48:43Z

Oh my god, this was already working in release branch why you didn't merge it?

aaronjudd · 2018-03-02T16:09:34Z

@DenisBogatirov there's still breaking tests and some conflicts that needed cleaning up. Originally we weren't going to release this until the 2.0 release (big changes), but as it's looking pretty close, I've tagged this for 1.9 release (next week), so hopefully it'll be merged pretty quickly. @aldeed is actually on vacation though, so waiting on him to return, before we do a final review and merge.

pmn4 · 2018-03-05T11:28:35Z

the merge conflicts in shop.js look pretty straightforward (and conflict with some changes that I made), so happy to lend a hand resolving

spencern

Conflicts resolved, tests passed. Merging.

aaronjudd · 2018-03-07T04:39:20Z

@aldeed new issue found (after merge into 1.9) -> can create a new issue but thought it'd be useful here:

Seen clicking on "Tax" in sidebar, or opening any of the tax settings:

meteor.js?hash=b0f12795c8cc1423b5850502871996903f947ed5:992 TypeError: fieldSchema.type[0].type is not a function
    at Form.renderField (form.js:267)
    at form.js:316
    at Array.map (<anonymous>)
    at Form.renderWithSchema (form.js:304)
    at Form.render (form.js:344)
    at finishClassComponent (modules.js?hash=4f2eb55914217a0342d190203eac0918f9672a78:186673)
    at updateClassComponent (modules.js?hash=4f2eb55914217a0342d190203eac0918f9672a78:186650)
    at beginWork (modules.js?hash=4f2eb55914217a0342d190203eac0918f9672a78:187025)
    at performUnitOfWork (modules.js?hash=4f2eb55914217a0342d190203eac0918f9672a78:189024)
    at workLoop (modules.js?hash=4f2eb55914217a0342d190203eac0918f9672a78:189088)```

aldeed force-pushed the aldeed-npm-simple-schema branch 3 times, most recently from 8078627 to 51c4b7d Compare December 1, 2017 00:07

brent-hoover requested a review from a team December 1, 2017 00:11

brent-hoover self-requested a review December 1, 2017 01:01

aaronjudd reviewed Dec 7, 2017

View reviewed changes

aldeed force-pushed the aldeed-npm-simple-schema branch from 0c2a772 to bb622a6 Compare December 20, 2017 03:08

aldeed added 5 commits December 19, 2017 21:10

Use NPM SimpleSchema rather than Meteor

0b390ed

WIP Latest changes

49dd908

Fix apps dashboard display issue

b6a52ad

Fix form generation issue

8946110

Fix more simpl-schema bugs

982f9c1

aldeed force-pushed the aldeed-npm-simple-schema branch from bb622a6 to 982f9c1 Compare December 20, 2017 03:11

Fix tests

4407bb6

aldeed commented Dec 30, 2017

View reviewed changes

Remove link

8eb329b

brent-hoover reviewed Jan 1, 2018

View reviewed changes

brent-hoover reviewed Jan 2, 2018

View reviewed changes

Address some of @zenweasel PR review

f5f2150

Merge branch 'master' into aldeed-npm-simple-schema

6b0f931

brent-hoover previously approved these changes Jan 30, 2018

View reviewed changes

machikoyasuda mentioned this pull request Feb 5, 2018

(doc): add pull request template reactioncommerce/reaction-docs#465

Merged

Merge branch 'release-1.8.0' into aldeed-npm-simple-schema

4ca56d0

aldeed dismissed brent-hoover’s stale review via 4ca56d0 February 5, 2018 18:45

aldeed changed the base branch from master to release-1.8.0 February 7, 2018 17:14

Merge branch 'master' into aldeed-npm-simple-schema

3bb3308

aldeed changed the base branch from release-1.8.0 to master February 23, 2018 04:46

chore: Switch to collection2 pkg

0a1863f

aldeed mentioned this pull request Feb 23, 2018

docs(schemas): Updates for switch to NPM SimpleSchema reactioncommerce/reaction-docs#529

Merged

aaronjudd requested a review from spencern March 2, 2018 16:06

aldeed added 3 commits March 6, 2018 10:16

Merge branch 'master' into aldeed-npm-simple-schema

7ccbe9c

tests: Fix test so that the real error is seen rather than timeout

35d98ba

fix: Use latest schema-index to avoid loading collection2 twice

91b3a66

spencern changed the base branch from master to release-1.9.0 March 7, 2018 01:41

aldeed added 2 commits March 6, 2018 20:05

Merge branch 'release-1.9.0' into aldeed-npm-simple-schema

dcb5926

chore: Resolve lint issues

ba46dd2

spencern changed the title ~~Use NPM SimpleSchema rather than Meteor~~ Use NPM SimpleSchema rather than Meteor Mar 7, 2018

spencern approved these changes Mar 7, 2018

View reviewed changes

spencern merged commit c5abc7f into release-1.9.0 Mar 7, 2018

spencern deleted the aldeed-npm-simple-schema branch March 7, 2018 03:36

aldeed mentioned this pull request Mar 7, 2018

Handle integer schema type when getting form field type #3930

Merged

spencern mentioned this pull request Mar 8, 2018

Release 1.9.0 #3941

Merged

		@@ -1,6 +1,18 @@
		import { Meteor } from "meteor/meteor";
		import SimpleSchema from "simpl-schema";

Use NPM SimpleSchema rather than Meteor #3331

Use NPM SimpleSchema rather than Meteor #3331

Conversation

aldeed commented Nov 20, 2017 • edited Loading

brent-hoover commented Dec 1, 2017

aldeed commented Dec 1, 2017

aaronjudd left a comment

Choose a reason for hiding this comment

aldeed commented Dec 11, 2017

aldeed 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

aldeed commented Dec 30, 2017

brent-hoover commented Jan 1, 2018

brent-hoover commented Jan 1, 2018

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

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

brent-hoover commented Jan 2, 2018 • edited Loading

aaronjudd commented Jan 4, 2018

brent-hoover commented Jan 5, 2018

aldeed commented Jan 8, 2018

brent-hoover commented Jan 30, 2018

brent-hoover left a comment

Choose a reason for hiding this comment

DenisBogatirov commented Feb 5, 2018 • edited Loading

aldeed commented Feb 5, 2018

DenisBogatirov commented Mar 2, 2018

aaronjudd commented Mar 2, 2018

pmn4 commented Mar 5, 2018

spencern left a comment

Choose a reason for hiding this comment

aaronjudd commented Mar 7, 2018 • edited Loading

aldeed commented Nov 20, 2017 •

edited

Loading

brent-hoover commented Jan 2, 2018 •

edited

Loading

DenisBogatirov commented Feb 5, 2018 •

edited

Loading

aaronjudd commented Mar 7, 2018 •

edited

Loading