Native functions #2309

gabrielsimongianotti · 2021-10-31T22:25:18Z

Description

i make two alters of #2187 (comment), removed lodash/isPlainObject, lodash/isObject and replace per native function

How Has This Been Tested?

I used your tests

Types of changes

remove lodash/isPlainObject
remove lodash/isObject

Checklist:

isObject
sPlainObject

char0n · 2021-11-02T08:45:07Z

src/execute/index.js

@@ -76,7 +75,14 @@ export function execute({
    ...extras,
  });

-  if (request.body && (isPlainObject(request.body) || Array.isArray(request.body))) {


Comparing this change with original, I'd say we'll stick with isPlainObject for the time being. Determining if something is POJO is a hard job, and having a well tested utility function that abstract the complexity just make sense.

I think we have two options with isPlainObject
1.) remove the isPlainObject from the list and keep using lodash version
2.) create our own utility function that does exactly the same thing as isPlainObject from lodash

The scope of #2187 (as I understood it) is to replace lodash functions with modern & native simple javascript constructs. IMHO this doesn't fall within that scope.

// cc @jimmywarting

What @char0n have mention about isPlainObject not being equivalent to just a simple typeof is true... need to check more things such as function, null etc

in some senarios it's fine to use just typeof x === object but we need to know for sure if it's okey to use it.

the point of my issue is not to use any lodash at all, even if you just include one simple function then lodash will at least load some internal core functions that are pretty big in itself and is deeply dependent on many other stuff to make it compatible with es3 or 5 (whatever it is).

Right now there is so few lodash fn that we use that i don't see any point in keeping it, so i suggest # 2
create our own utility function that does the same thing

where can i create function isPlainObject? In which project folder?

@gabrielsimongianotti src/helpers.js contains already two helper functions at the top of the file. Le'ts put isPlainObject

Another options is to use https://www.npmjs.com/package/is-plain-object

Contains the logic that we need and handles corner cases. Has no dependencies as well.

If there are no strong objections to this proposal let's consider this package as a replacement for lodash.isPlainObject

char0n · 2021-11-02T08:51:20Z

src/helpers.js

@@ -136,7 +134,7 @@ export function normalizeSwagger(parsedSpec) {
  for (const pathName in paths) {
    const path = paths[pathName];

-    if (!isObject(path)) {
+    if (typeof path !== 'object') {


isObject cannot be simply replaced with typeof path === 'object'; this is not functionally equivalent.

The source code of isObject is as follows:

function isObject(value) { var type = typeof value; return value != null && (type == 'object' || type == 'function'); }

Which means that anything that reports typeof object || function except null and undefined.

char0n · 2021-11-02T08:54:23Z

src/helpers.js

@@ -136,7 +134,7 @@ export function normalizeSwagger(parsedSpec) {
  for (const pathName in paths) {
    const path = paths[pathName];

-    if (!isObject(path)) {
+    if (typeof path !== 'object') {


Suggested change

if (typeof path !== 'object') {

if (path == null || !['object', 'function'].includes(typeof path)) {

char0n · 2021-11-02T08:54:42Z

src/helpers.js

@@ -145,7 +143,7 @@ export function normalizeSwagger(parsedSpec) {
    // eslint-disable-next-line no-restricted-syntax, guard-for-in
    for (const method in path) {
      const operation = path[method];
-      if (!isObject(operation)) {
+      if (typeof path !== 'object') {


Suggested change

if (typeof path !== 'object') {

if (path == null || !['object', 'function'].includes(typeof path)) {

char0n

Thanks for contributing! I've added couple of code review comments. Please have a look at them.

src/execute/oas3/build-request.js

char0n · 2021-11-05T15:40:53Z

Made one change, but the PR is missing changes in package-lock.json file, after we've introduced new library.

gabrielsimongianotti · 2021-11-05T17:08:24Z

Made one change, but the PR is missing changes in package-lock.json file, after we've introduced new library.

thank you to remember 😅

char0n

As we've introduced new library, we need the package-lock.json file change in this PR as well.

gabrielsimongianotti · 2021-11-08T21:23:41Z

As we've introduced new library, we need the package-lock.json file change in this PR as well.

I maked merge in 'upstream/master' , is something missing? you can make approve?

char0n · 2021-11-09T07:53:31Z

As we've introduced new library, we need the package-lock.json file change in this PR as well.

I maked merge in 'upstream/master' , is something missing? you can make approve?

You need to run following command in repo fork:

 $ npm i --save is-plain-object

This will modify two files: package.json and package-lock.json. These files needs to be part of this PR. We have changes in package.json but you didn't submitted changes in package-lock.json.

More info about package-lock files here: https://docs.npmjs.com/cli/v7/configuring-npm/package-lock-json

gabrielsimongianotti · 2021-11-10T13:21:42Z

As we've introduced new library, we need the package-lock.json file change in this PR as well.

I maked merge in 'upstream/master' , is something missing? you can make approve?

You need to run following command in repo fork:
 $ npm i --save is-plain-object
This will modify two files: package.json and package-lock.json. These files needs to be part of this PR. We have changes in package.json but you didn't submitted changed in package-lock.json.

More info about package-lock files here: https://docs.npmjs.com/cli/v7/configuring-npm/package-lock-json

sorry i don't speak english very well, now i understand what you were trying to say

char0n · 2021-11-10T15:15:01Z

I've made some final changes to the PR, regarding package-lock.json file. We're using lockfileVersion: 1, you've generated it for lockfileVersion: 2. No big deal, changed it back to v1.

char0n · 2021-11-10T15:15:14Z

Merging and thanks for contributing!

swagger-bot · 2021-12-21T10:12:12Z

🎉 This PR is included in version 3.18.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

gabrielsimongianotti added 2 commits October 31, 2021 16:45

refactor: remove isObject by native function

b442f2a

refactor: remove isPlainObject by native function

2f79cd9

char0n reviewed Nov 2, 2021

View reviewed changes

char0n requested changes Nov 2, 2021

View reviewed changes

gabrielsimongianotti added 2 commits November 2, 2021 21:12

feat: alter condition of if

e1c04cc

fix: remove a parenthesis not necessary

cfc569f

gabrielsimongianotti requested a review from char0n November 5, 2021 04:05

gabrielsimongianotti added 2 commits November 5, 2021 12:19

feat: add is-plain-object

248038b

refactor: implatation isPlainObject

5c8c3dc

char0n reviewed Nov 5, 2021

View reviewed changes

src/execute/oas3/build-request.js Outdated Show resolved Hide resolved

Update src/execute/oas3/build-request.js

9f2c14b

Merge remote-tracking branch 'upstream/master' into native-functions

869680a

gabrielsimongianotti requested a review from char0n November 5, 2021 17:08

char0n requested changes Nov 8, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into native-functions

ca2304c

gabrielsimongianotti requested a review from char0n November 8, 2021 21:20

feat: add is-plain-object

a11475d

char0n added 2 commits November 10, 2021 15:59

Update package-lock.json

3ab9b7a

Merge branch 'master' into native-functions

2314c13

char0n approved these changes Nov 10, 2021

View reviewed changes

char0n added 2 commits November 10, 2021 16:07

Update package.json

7ec3857

Update package.json

7cd1c16

Update package-lock.json

dc67208

char0n merged commit 067229e into swagger-api:master Nov 10, 2021

swagger-bot added the released label Dec 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Native functions #2309

Native functions #2309

gabrielsimongianotti commented Oct 31, 2021

char0n Nov 2, 2021 •

edited

Loading

jimmywarting Nov 2, 2021

gabrielsimongianotti Nov 2, 2021

char0n Nov 5, 2021

char0n Nov 5, 2021 •

edited

Loading

char0n Nov 2, 2021

char0n Nov 2, 2021

char0n Nov 2, 2021

char0n left a comment

char0n commented Nov 5, 2021

gabrielsimongianotti commented Nov 5, 2021

char0n left a comment

gabrielsimongianotti commented Nov 8, 2021

char0n commented Nov 9, 2021 •

edited

Loading

gabrielsimongianotti commented Nov 10, 2021

char0n commented Nov 10, 2021

char0n commented Nov 10, 2021

swagger-bot commented Dec 21, 2021

	if (typeof path !== 'object') {
	if (path == null \|\| !['object', 'function'].includes(typeof path)) {

Native functions #2309

Native functions #2309

Conversation

gabrielsimongianotti commented Oct 31, 2021

Description

How Has This Been Tested?

Types of changes

Checklist:

char0n Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

jimmywarting Nov 2, 2021

Choose a reason for hiding this comment

gabrielsimongianotti Nov 2, 2021

Choose a reason for hiding this comment

char0n Nov 5, 2021

Choose a reason for hiding this comment

char0n Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

char0n Nov 2, 2021

Choose a reason for hiding this comment

char0n Nov 2, 2021

Choose a reason for hiding this comment

char0n Nov 2, 2021

Choose a reason for hiding this comment

char0n left a comment

Choose a reason for hiding this comment

char0n commented Nov 5, 2021

gabrielsimongianotti commented Nov 5, 2021

char0n left a comment

Choose a reason for hiding this comment

gabrielsimongianotti commented Nov 8, 2021

char0n commented Nov 9, 2021 • edited Loading

gabrielsimongianotti commented Nov 10, 2021

char0n commented Nov 10, 2021

char0n commented Nov 10, 2021

swagger-bot commented Dec 21, 2021

char0n Nov 2, 2021 •

edited

Loading

char0n Nov 5, 2021 •

edited

Loading

char0n commented Nov 9, 2021 •

edited

Loading