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

Replace third party deep-equal library with native implementation #5509

Merged
merged 3 commits into from
Jul 31, 2020

Conversation

Fawke
Copy link
Contributor

@Fawke Fawke commented Jul 17, 2020

Type of change

  • Utils Functions

Description of change

deep-equal module is replaced by a native implementation for deeply checking object equality.

This function is mainly used by the sizeMappingV2.js module, which is tested and working fine after the replacement. This implementation is simple, but should mostly work for our use cases.

Also, prebid- core.js is now almost 40kb lighter after removal of this module.

Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed a few places where deep-equal should be removed (such as the whitelist): https://github.com/prebid/Prebid.js/search?q=deep-equal&unscoped_q=deep-equal

other than that, looks good

Copy link
Contributor

@idettman idettman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than what Rich mentioned, LGTM

@Fawke
Copy link
Contributor Author

Fawke commented Jul 27, 2020

@snapwich @idettman Thanks for the review.

@snapwich, Good catch, I forgot to replace the reference from allowedModules.js. One place, where I can not remove it from is fake-responder.js file.

It runs in node environment and throws an error if I try to require the utils.js file because the utils file uses import statements which are not recognized by node. I tried few solutions, like:

  • using the mjs extension on fake-responder.js file
  • trying to include node environment as target in babelrc.js file
  • adding type: "module" key in parent package.json file

None of these approaches work. So, in the end, I had to add back the deep-equal library, but this time as a devDependency which will only be used in fake-responder.js file. Util.js will still have the native implemenation and we still achieve reduction in overall size of prebid-core webpack chunk.

If you have any better solution to how I can import utils in a file running in node env, let me know, or we can merge this.

@idettman
Copy link
Contributor

idettman commented Jul 27, 2020

It runs in node environment and throws an error if I try to require the utils.js file because the utils file uses import statements which are not recognized by node. I tried few solutions, like:

  • using the mjs extension on fake-responder.js file
  • trying to include node environment as target in babelrc.js file
  • adding type: "module" key in parent package.json file

Imports and exports support In Node.js 12 requires both:

  1. Passing the --experimental-modules flag when running Node.js
  2. using .mjs extension or set "type": "module" in your package.json.

I'm guessing you're missing the --experimental-modules flag step, but since we are using gulp, we may need to add a environment variable (NODE_OPTIONS="--experimental-modules") to set the flag.

@Fawke
Copy link
Contributor Author

Fawke commented Jul 29, 2020

@idettman I tried the approach that you suggested, using the flag --experimental-modules

  1. setting extension to .mjs doesn't work because then, we would need to change the extension of all the files which are es modules to .mjs, which would require us to change extension of utils.js file and all the files imported by it.

  2. setting type, in package.json to module also creates some problem. In some places in the code, we are still using require instead of import. This will require us to remove all references to require which will be an elaborate change, deserving its own PR.

We can copy the implementation of deepEqual from utils to a separate function in fake-response.js file, or use the third party deep-equal library but only as a devDependency. Let me know your thoughts.

Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think moving deep-equal to a devDependency is perfectly acceptable

@Fawke Fawke merged commit 477fe0c into master Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants