-
-
Notifications
You must be signed in to change notification settings - Fork 58
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
Add buildManagerOptions
config option (supports opting out of --ignore-lockfile
)
#409
Conversation
Codecov Report
@@ Coverage Diff @@
## master #409 +/- ##
==========================================
+ Coverage 96.72% 96.87% +0.14%
==========================================
Files 17 17
Lines 550 576 +26
==========================================
+ Hits 532 558 +26
Misses 18 18
Continue to review full report at Codecov.
|
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.
Instead of adding another flag, we really want a way to specify exactly what arguments to use with the package manager. I think that would allow more overall capabilities and be easier in the implementation.
Concretely, I think this would be a decent API:
// config/ember-try.js
module.exports = async function() {
return {
buildNpmOptions(scenarioName) {
return []; // opt-out of defaulting `--ignore-engines --no-lockfile`
},
scenarios: [
}
};
};
Then we could structure dependency-manager-adapters/npm.js
, such that in _install
it would call the users function to get the final list of options.
I chatted with @kategengler, she suggested we use |
I see...so if someone has buildManagerOptions in their config, then we use that config instead of the default. If shrinkwrap or yarn.lock files are detected, then we back up those files and restore accordingly. I can make that change. |
Ya, exactly |
if (mgrOptions.indexOf('--ignore-engines') === -1) { | ||
mgrOptions = mgrOptions.concat(['--ignore-engines']); | ||
} else { | ||
if (this.useYarnCommand) { |
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.
Do we intend to mandate the buildManagerOptions in the future? If so, we can add a deprecation here that encourages to use the manager options for these defaults.
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.
Do we intend to mandate the buildManagerOptions in the future?
No, I believe that it will always be a "power user" option. I would expect that most users would use whatever our default command line options are, and you'd only specify buildManagerOptions
if you had to deviate from that for some reason.
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.
I agree; it is backward compatible and users do not need to use it if there isn't a need.
if (mgrOptions.indexOf('--ignore-engines') === -1) { | ||
mgrOptions = mgrOptions.concat(['--ignore-engines']); | ||
} else { | ||
if (this.useYarnCommand) { |
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.
Do we intend to mandate the buildManagerOptions in the future?
No, I believe that it will always be a "power user" option. I would expect that most users would use whatever our default command line options are, and you'd only specify buildManagerOptions
if you had to deviate from that for some reason.
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.
The code changes here look good to me, I think the only things left are documentation changes (update the README.md to include this option and explain what it does) and a review pass by @kategengler.
@kategengler @rwjblue Addressed all comments and added buildManagerOptions to the readme. |
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.
Awesome, thank you!
buildManagerOptions
config option (supports opting out of --ignore-lockfile
)
Released in v1.3.0. |
Currently, ember-try is running yarn with the --no-lockfile option, which will install packages whose version may differ from the actual current yarn.lock file in the project. To get a true dependency environment during testing, this PR aims to add support for using the project's yarn.lock file when using ember-try by adding a config flag called useYarnLockFile that users can pass in to determine whether to use the project's yarn.lock file.