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

AO-18716 fix node 14 non-existent property warning #139

Merged
merged 2 commits into from
Feb 11, 2021

Conversation

cheempz
Copy link
Contributor

@cheempz cheempz commented Feb 9, 2021

@bdehamer ran into the following error with our agent version 10.0.0 on Node 14.15

(node:1) Warning: Accessing non-existent property 'lambda' of module exports inside circular dependency
  at emitCircularRequireWarning (internal/modules/cjs/loader.js:650:11)
  at Object.get (internal/modules/cjs/loader.js:664:5)
  at module.exports (/app/node_modules/appoptics-apm/lib/api.js:37:10)
  at Object.<anonymous> (/app/node_modules/appoptics-apm/lib/index.js:372:53)
  at Module._compile (internal/modules/cjs/loader.js:1063:30)
  at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
  at Module.load (internal/modules/cjs/loader.js:928:32)
  at Function.Module._load (internal/modules/cjs/loader.js:769:14)
  at Module.require (internal/modules/cjs/loader.js:952:19)
  at Module._preloadModules (internal/modules/cjs/loader.js:1217:12)

Seems due to nodejs/node#29935 introduced in Node 14, and some examples of how other packages fixed it can be found in nodejs/node#32987

@cheempz cheempz requested a review from prettycoder February 9, 2021 02:30
@cheempz cheempz changed the title Fix node 14 non-existent warning Fix node 14 non-existent property warning Feb 9, 2021
@bdehamer
Copy link

bdehamer commented Feb 9, 2021

@cheempz I tested this change out and it does seem to resolve the issue I was seeing. There may be a few more of these to update though . . . I noticed that if I omitted the APPOPTICS_SERVICE_KEY value I would get the following:

(node:25) Warning: Accessing non-existent property 'cls' of module exports inside circular dependency
    at emitCircularRequireWarning (internal/modules/cjs/loader.js:650:11)
    at Object.get (internal/modules/cjs/loader.js:664:5)
    at module.exports (/app/node_modules/appoptics-apm/lib/api-sim.js:10:12)
    at Object.<anonymous> (/app/node_modules/appoptics-apm/lib/index.js:372:53)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Module.load (internal/modules/cjs/loader.js:928:32)
    at Function.Module._load (internal/modules/cjs/loader.js:769:14)
    at Module.require (internal/modules/cjs/loader.js:952:19)
    at Module._preloadModules (internal/modules/cjs/loader.js:1217:12)

@cheempz
Copy link
Contributor Author

cheempz commented Feb 9, 2021

Thanks for catching that @bdehamer! Could I prevail on you to give this one more try?

Copy link

@bdehamer bdehamer left a comment

Choose a reason for hiding this comment

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

These two changes appear to have fixed the warnings I was seeing 👍🏼

@cheempz cheempz changed the title Fix node 14 non-existent property warning AO-18716 fix node 14 non-existent property warning Feb 9, 2021
Copy link
Contributor

@prettycoder prettycoder left a comment

Choose a reason for hiding this comment

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

Thanks for figuring this out
lgtm
I added a suggestion, but it is not crucial

cls = ao.cls;
// check for property to avoid triggering node 14 warning on
// non-existent property in unfinished module.exports
if (ao.hasOwnProperty('cls')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

a suggestion:
if ('cls' in ao) { seems to be the pattern used in this code base for testing properties
hasOwnProperty is a bit more restrictive, as it would return false for inherited properties
Less restrictive may be more future-proof

Copy link

Choose a reason for hiding this comment

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

I don't have any insight into which approach is preferable, but can confirm that the 'cls' in ao style also prevents the warning from appearing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know :). I did consider the property in object style checking but decided against it since these two properties are very specific to the agent and I didn't see a scenario where the agent would try inheriting them from some parent object.

if (ao.lambda) {
// use property check that does not trigger node 14 warning on
// non-existent property in unfinished module.exports
if (ao.hasOwnProperty('lambda')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • dito -

@cheempz cheempz merged commit 051453a into master Feb 11, 2021
@cheempz cheempz deleted the ao-18716-fix-node14-warning branch February 11, 2021 19:27
@cheempz cheempz mentioned this pull request Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants