V7 merge

[pouya-eghbali]

Merge pouya-eghbali/meteor-autoform

[lgtm-com[bot]]

This pull request fixes 1 alert when merging 06a875b99031b32465ac75efc71561b41d8755c0 into d8f8b6a6e36079c054976ca8bda7bec97f548b4f - view on LGTM.com

fixed alerts:

• 1 for Prototype pollution in utility function

[jankapunkt]

So I cloned this and built it into my staging environment after locally were no issues. Our staging users reported no issues with it yet, so it seems to be a good fit :+1:

The only issues so far are from my end:

• we need to update the documentation
• we need to show how to add Bootstrap 4 templates, mainly because the imajus:autoform-bootstrap4 is relying on some Template helpers from the Bootstrap3 templates, and I had to create an own package that fixes the (jkuester:autoform-bootstrap4)
• we should at least add some unit tests for functions that don't require templating, puppeteer etc. for now (later we may extend this)
• I have tested some of my extensions and they all worked, however the most popular extensions need to be tested, too
• The extensions list may be curated in order to remove abandoned packages or get them into the community if they are very popular

I would offer to review the documentation but the @Meteor-Community-Packages/autoforms team may clone this and try it out locally, too.

[pouya-eghbali]

@jankapunkt sorry I'm really really busy these days with everything going remote, I don't know when I can help, maybe in two weeks, but do tell me if you find any issues. Personally I didn't have any issues on my dev machine, staging and on production so far and I don't have any updates for this PR for now.

[pouya-eghbali]

The var,indexOf and single char / short names are not my doings, I tried to use const whenever I rewrote any part of this, and if that wasn't possible I used let, I tried my best not to touch the original code but if we're doing a major update we should replace all these and move to >es6.

For the breaking of parameters and formatting, I used prettier with default settings, we can decide on a linter and add a config file to automatically format the code. I used prettier with default settings because that's what I use for all my projects.

All changes I made are somehow documented in the new readme, this was fine for a fork, but we need to move it to the documentation. Regarding the isObject function, do you know a standalone fast implementation?

[pouya-eghbali]

For isObject the current implementation is

export const isObject = obj => toString.call(obj) == '[object Object]' || Array.isArray(obj) || isFunction(obj)

can't remember the reason behind this, we should check if removing || Array.isArray(obj) || isFunction(obj) won't break any functionality, or if it does then it should be replaced with an appropriate function call where it breaks.

[jankapunkt]

The var,indexOf and single char / short names are not my doings, I tried to use const whenever I rewrote any part of this, and if that wasn't possible I used let, I tried my best not to touch the original code but if we're doing a major update we should replace all these and move to >es6.

For the breaking of parameters and formatting, I used prettier with default settings, we can decide on a linter and add a config file to automatically format the code. I used prettier with default settings because that's what I use for all my projects.

I see, I was already curious because at some parts there wire mixed var and const. I think prettier is a stable and well established tool and we could go for it. I could setup some github action / ci using it to lint-test any pushes and pull requests (will create a PR for that).

If you give me access to the dev repo I can take over that part with the ES6 and fix all my mentioned things in the review. An alternative would be to merge this into a v7.x branch instead of devel and work from here towards a stable v7. What do you think?

can't remember the reason behind this, we should check if removing || Array.isArray(obj) || isFunction(obj) won't break any functionality, or if it does then it should be replaced with an appropriate function call where it breaks.

I think the function should not be too complicated and fulfill the following requirements:

• check if it's not an Array
• check if it's not a Function

I checked the polyfill for Array.isArray and would do

export const isObject = obj => Object.prototype.toString.call(obj) === '[object Object]'

which is basically the explicit version of toString.call(obj) == '[object Object]' and in any situation it breaks there should be an extra condition with else if (Array.isArray(obj)) { ... } and/or `else if (typeof obj === 'function') { ... }```

[jankapunkt]

I just want to ask again, do you think it's a good idea to merge this to a v7 branch instead of develop so I can immediately fix the issues that are left and we can soon merge? When I am through I will ask for at least one reviewer from the team and then we should soon be able to merge.

[pouya-eghbali]

Yeah, let's do that. That sounds like a better idea.

[jankapunkt]

Allright, changed the base branch and will implement the mentioned issues, I hope this will be done this weekend :-)