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

Deprecate aldeed:simple-schema and implement npm simpl-schema #2917

Closed
aaronjudd opened this issue Sep 21, 2017 · 8 comments
Closed

Deprecate aldeed:simple-schema and implement npm simpl-schema #2917

aaronjudd opened this issue Sep 21, 2017 · 8 comments
Assignees
Milestone

Comments

@aaronjudd
Copy link
Contributor

Deprecate https://github.com/aldeed/meteor-simple-schema and replace with https://github.com/aldeed/node-simple-schema

We've tested a few times and there were some issues, but it's time to revisit, as we need this in place for our react components to use for validation, and also in some new registry requirements we're working on. This would also allow us to deprecate meteor autoform as soon as the remaining dashboard blaze templates are converted.

@aaronjudd aaronjudd added meteor help wanted For issues that have a clear solution described and are not currently prioritized core team work labels Sep 21, 2017
@brent-hoover
Copy link
Collaborator

fwiw I played with this recently and the main issue I ran into was that node-simple-schema does not support things like [MyCustomSchema] which we have a fair amount of.

@spencern
Copy link
Contributor

spencern commented Oct 6, 2017

I've gone ahead and replaced all instances of type: [String] or any other arrays that are built like that in our current code base.
See my branch spencer-node-simple-schema-update-schemas

Current issues I'm running into:

it appears that the node version does not support our index property

Error: Invalid definition for userId field: "index" is not a supported property
    at /Users/sn/dev/reaction-dev/node_modules/simpl-schema/dist/SimpleSchema.js:965:13
    at Function._.each._.forEach (/Users/sn/dev/reaction-dev/node_modules/underscore/underscore.js:158:9)
    at checkAndScrubDefinition (/Users/sn/dev/reaction-dev/node_modules/simpl-schema/dist/SimpleSchema.js:963:24)
    at /Users/sn/dev/reaction-dev/node_modules/simpl-schema/dist/SimpleSchema.js:498:9
    at Function._.each._.forEach (/Users/sn/dev/reaction-dev/node_modules/underscore/underscore.js:158:9)
    at SimpleSchema.extend (/Users/sn/dev/reaction-dev/node_modules/simpl-schema/dist/SimpleSchema.js:486:28)
    at new SimpleSchema (/Users/sn/dev/reaction-dev/node_modules/simpl-schema/dist/SimpleSchema.js:107:10)
    at meteorInstall.lib.collections.schemas.accounts.js (lib/collections/schemas/accounts.js:74:25)
    at fileEvaluate (packages/modules-runtime.js:333:9)
    at require (packages/modules-runtime.js:228:16)
=> Exited with code: 1

From @mikemurray, we need to be aware of how our Validation works with this. I've not looked into this at all yet.

I've not had a chance to update main.js yet either.

Because the app crashes immediately, I can't consider this to be an entire list of issues we'll run into when completing the swap, but it's definitely a good start.

For 1.5.0 we're going to launch with registerSchema in order to fix the load order issues were having right now. @kieckhafer thinks we may need to do this even with the node version, that seems like a reasonable argument to make after my time digging around in it.

@spencern
Copy link
Contributor

spencern commented Oct 6, 2017

Related #3034

@kieckhafer
Copy link
Member

PR #3053 will be the registerSchema updates. It's working well as is with a few test schemas, just need to make sure all schemas are registered.

@aldeed
Copy link
Contributor

aldeed commented Nov 7, 2017

@aaronjudd @spencern Let me know if you want me to tackle this and #3034. I'm going to try getting back into the reaction codebase a bit.

@aaronjudd
Copy link
Contributor Author

aaronjudd commented Nov 7, 2017

@aldeed that'd be awesome! happy to have you. (added you back to collaborators to make life easy)

@spencern
Copy link
Contributor

spencern commented Nov 7, 2017

Would love that @aldeed
I've got a branch that has a decent start (updates all arrays in our schema to fit the node-simple-schema model) if you want a head start.

https://github.com/reactioncommerce/reaction/tree/spencer-node-simple-schema-update-schemas

@aldeed
Copy link
Contributor

aldeed commented Nov 7, 2017

@spencern Thanks, I will start from there

@brent-hoover brent-hoover removed help wanted For issues that have a clear solution described and are not currently prioritized core team work ready labels Nov 15, 2017
@spencern spencern added this to the Performance Sprint 1 milestone Jan 25, 2018
@spencern spencern modified the milestones: Performance Sprint 1, Release 2.0 Feb 20, 2018
@spencern spencern closed this as completed Mar 8, 2018
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

No branches or pull requests

5 participants