script/redownload-electron-bins.js: Fix finding the Atom repo's package.json#2534
Conversation
Incorrect capitalization caused this line to reference a non-existent/undefined variable.
Construct the atomRepoPath variable starting with __dirname, so that atomRepoPath is an absolute path. For example: '/Users/[username]/atom/package.json', versus '../../../../atom/package.json'. --- Explanation for why this is necessary: The require() and fs.existsSync() functions interpret relative paths differently. require() is relative to the current script's containing folder, (which is the same as __dirname), whereas 'fs.existsSync()' is relative to the working directory (which is the same as process.cwd()). When constructing the path to the Atom repo's main package.json, we should construct it as an absolute path, for consistency's sake. Otherwise the script will error out, or use the wrong package.json.
|
cc @smashwilson who reviewed the initial PR for this script. This script from #2513 has a nifty feature that makes the This PR is to fix those two small issues. Please take a look if you get the chance. Thank you. |
| const atomRepoPath = path.join('..', '..', '..', '..', 'atom', 'package.json'); | ||
| const electronVersion = fs.existsSync(atomRepoPath) ? require(atomrepoPath).electronVersion : '6.1.12' | ||
| const atomRepoPath = path.join(__dirname, '..', '..', '..', '..', 'atom', 'package.json'); | ||
| const electronVersion = fs.existsSync(atomRepoPath) ? require(atomRepoPath).electronVersion : '6.1.12' |
There was a problem hiding this comment.
Off-topic, but it might make sense to call this variable atomMetadata or atomMetadataPath instead of atomRepoPath. That's more consistent with how Atom's scripts refer to package.json files (as "metadata").
DeeDeeG
changed the title
DeeDeeG
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2534 +/- ##
=======================================
Coverage 93.43% 93.43%
=======================================
Files 236 236
Lines 13204 13204
Branches 1899 1899
=======================================
Hits 12337 12337
Misses 867 867 Continue to review full report at Codecov.
|
smashwilson
left a comment
smashwilson
left a comment
There was a problem hiding this comment.
Ah... good catch 👍🏻 Thanks for the follow-up!
3 participants
Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.
Requirements
Description of the Change
package.jsonas an absolute path, becausefs.existsSync()andrequire()interpret relative paths differently.require([path/to/Atom's/package.json])would have errored out.Screenshot/Gif
N/A
Alternate Designs
N/A. These are just typo/bug fixes. The overall design was reviewed in #2513, but this small typo and bug slipped past my/the reviewer's radars at the time.
Benefits
Fixes the original intended behavior of
script/redownload-electron-bins.js:When installing this package as a dependency in the Atom repo, and running this package's tests in that context, this package now installs/tests against the version of Electron specified in Atom's
package.json. Should provide more relevant results when running the package tests in Atom's CI.(Was falling back on a "fallback" version hard-coded in
script/redownload-electron-bins.js.)Possible Drawbacks
N/A, these are just bug-fixes.
(I don't think I missed anything this time! This is a very small change, only 12 characters of diff.)
Applicable Issues
Follow-up to #2513
Metrics
N/A
Tests
Steps to confirm that this change works (click to expand):
Clone this repo (with fix), positioned as if it were a dependency in the Atom repo:
cdinto the Atom repo.githubdependency if already installed.mv node_modules/github node_modules/_github_backupnode_modulesdircd node_modules && git clone https://github.com/DeeDeeG/githubcd github && git checkout fix-redownload-electron-binsConfirm the fix is working:
githubfolder we just cloned, donpm installto install all dependencies and devDevpendenciespackage.jsonas the value forelectronVersion)electronVersionin Atom'spackage.jsonand runnpm installagain in thegithubfolderpackage.jsonmksnapshotbinary changes when instaling different major versionsls -lah ./node_modules/electron-mksnapshot/bin/mksnapshotConfirm this behavior doesn't work as intended without the fix from this PR:
masterbranch in thegithubfolder, and runnpm installagaingit checkout master && npm installscript/redownload-electron-bins.jsscript/redownload-electron-bins.jsand runnpm installagainscript/redownload-electron-bins.jsis always downloaded.I followed these steps, and I am able to confirm this fix makes things work as I originally intended them to.
Documentation
N/A
Release Notes
N/A
User Experience Research (Optional)
None.