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
[HERMES-3750] Fix bug where missing .js file results in JSON.parse error #1587
Conversation
a0cf5d9
to
b0af2c0
Compare
Codecov Report
@@ Coverage Diff @@
## next-gen #1587 +/- ##
===========================================
Coverage ? 80.64%
===========================================
Files ? 20
Lines ? 1767
Branches ? 502
===========================================
Hits ? 1425
Misses ? 221
Partials ? 121 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
b0af2c0
to
0ba4eef
Compare
@@ -78,34 +100,7 @@ function find(currentPath, targetFilename) { | |||
} | |||
} | |||
|
|||
function getManifestData(searchDir) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to separate file for easier module mocking
src/cli/hook-utils/manifest.js
Outdated
/** | ||
* Union merge of arrays | ||
*/ | ||
export function unionMerge(array1, array2) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srajiang it seems like this export
keyword here threw an error for me when running hermes manifest
while testing. i think since it's getting exported via module.exports
it should be good to remove this export
keyword!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
full error I got:
🚫 Unable to get your app manifest details. Please check your manifest file:
A script hook defined in the Slack Configuration file (`slack.json`) returned an error (sdk_hook_invocation_failed)
Error Details:
1: Command for 'manifest' returned an error: exit status 1
/Users/ashley.huynh/Documents/Forked/bolt-js/src/cli/hook-utils/manifest.js:8
export function unionMerge(array1, array2) {
^^^^^^
SyntaxError: Unexpected token 'export'
at Object.compileFunction (node:vm:360:18)
at wrapSafe (node:internal/modules/cjs/loader:1049:15)
at Module._compile (node:internal/modules/cjs/loader:1084:27)
at Module._extensions..js (node:internal/modules/cjs/loader:1174:10)
at Module.load (node:internal/modules/cjs/loader:998:32)
at Module._load (node:internal/modules/cjs/loader:839:12)
at Module.require (node:internal/modules/cjs/loader:1022:19)
at require (node:internal/modules/cjs/helpers:102:18)
at Object.<anonymous> (/Users/ashley.huynh/Documents/Forked/bolt-js/src/cli/hook-utils/get-manifest-data.js:7:5)
at Module._compile (node:internal/modules/cjs/loader:1120:14)
Node.js v18.7.0
Suggestion:
Try to run `slack doctor` to check that your system dependencies are up-to-date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @ashleymcm 🙇♀️ - I've updated to remove the unnecessary export
27871a8
to
91fffd1
Compare
@@ -16,12 +21,16 @@ function readManifestJSONFile(searchDir, filename) { | |||
manifestJSON = JSON.parse(fs.readFileSync(jsonFilePath, 'utf8')); | |||
} | |||
} catch (error) { | |||
return; | |||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now this is properly returning null
instead of undefined
…ror (#1587) * Handle missing imported manifest (.js) * Add tests for getManifestData()
…ror (#1587) * Handle missing imported manifest (.js) * Add tests for getManifestData()
…ror (#1587) * Handle missing imported manifest (.js) * Add tests for getManifestData()
…ror (#1587) * Handle missing imported manifest (.js) * Add tests for getManifestData()
Summary
Testing
npm link
&npm link @slack/bolt
hermes manifest
andhermes manifest validate
and a manifest should properly generateRequirements (place an
x
in each[ ]
)