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

Warning while bundling with esbuild #3325

Closed
3 tasks done
blacha opened this issue Jun 25, 2020 · 6 comments
Closed
3 tasks done

Warning while bundling with esbuild #3325

blacha opened this issue Jun 25, 2020 · 6 comments
Labels
bug This issue is a bug.

Comments

@blacha
Copy link

blacha commented Jun 25, 2020

Confirm by changing [ ] to [x] below to ensure that it's a bug:

Describe the bug
I recently switched to using https://github.com/evanw/esbuild to bundle some of my javascript lambdas & libs which include aws-sdk, when bundling I receive the following warning

node_modules/aws-sdk/lib/xml/node_parser.js:142:32: warning: Comparison using the === operator here is always false
  if (keys.length === 0 || keys === ['$']) {

Specifically, https://github.com/aws/aws-sdk-js/blob/master/lib/xml/node_parser.js#L142

Does x === ['$'] ever return true?

Is the issue in the browser/Node.js?
Node.js

If on Node.js, are you running this on AWS Lambda?
Both Lambda & Nodejs

Details of the browser/Node.js version
v14.4.0

SDK version number
2.699.0 & 2.704.0

To Reproduce (observed behavior)

yarn add aws-sdk esbuild
echo "require('aws-sdk')" > index.js
npx esbuild --bundle index.js --platform=node --outdir=dist 

Expected behavior
bundled without warnings

Screenshots
N/A

Additional context
N/A

@blacha blacha added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 25, 2020
@ajredniwja
Copy link
Contributor

Hey @blacha thank-you for reaching out to us with your issue.

The warning generated actually doesn't make sense to me as XML can be an Object too and it can be empty.
With empty object it will return.

https://github.com/aws/aws-sdk-js/blob/master/lib/xml/node_parser.js#L142

I think it should be an issue for https://github.com/evanw/esbuild

Feel free to reach out if you think differently.

@blacha
Copy link
Author

blacha commented Jun 26, 2020

@ajredniwja I think this is a issue with the node_parser.js and not esbuild.

Since === compares the identity of objects, === [anything] will never equal true

Some examples

['$'] === ['$'] // false

var x = []
x === [] // false
x === x // true

const foo = {$: 'bar'}
Object.keys(foo) === ['$'] // false

if the goal is to check if the object is ['$'] the check should be arr.length === 1 && arr[0] === '$' but is this the goal?

@mhart
Copy link
Contributor

mhart commented Sep 2, 2020

This shouldn't have been closed. @blacha is 100% correct here #3430

@trivikr
Copy link
Member

trivikr commented Sep 2, 2020

Reopening as part of reviewing PR #3430

if the goal is to check if the object is ['$'] the check should be arr.length === 1 && arr[0] === '$' but is this the goal?

I'll go through some history to answer this question

@trivikr trivikr reopened this Sep 2, 2020
@trivikr
Copy link
Member

trivikr commented Sep 2, 2020

The line in question is:

if (keys.length === 0 || keys === ['$']) {

The commit which added that line is c794ac5

From quick analysis, it appears that author wanted to check if array length is 1 and the key at index 0 is $ as explained by @blacha in their comment

@trivikr
Copy link
Member

trivikr commented Sep 2, 2020

Verified that esbuild is now able to bundle aws-sdk in v2.745.0 as follows:

$ yarn add aws-sdk esbuild

$ yarn list aws-sdk
yarn list v1.22.4
warning package.json: No license field
warning No license field
warning Filtering by arguments is deprecated. Please use the pattern option instead.
└─ aws-sdk@2.745.0
✨  Done in 0.10s.

$ echo "require('aws-sdk')" > index.js

$ ./node_modules/.bin/esbuild --bundle index.js --platform=node --outdir=dist

@trivikr trivikr closed this as completed Sep 2, 2020
@trivikr trivikr removed the needs-triage This issue or PR still needs to be triaged. label Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug.
Projects
None yet
Development

No branches or pull requests

4 participants