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

Add disabling of autoinstall globally via environment variable #2130

Closed
wants to merge 708 commits into from
Closed

Add disabling of autoinstall globally via environment variable #2130

wants to merge 708 commits into from

Conversation

cacheflow
Copy link
Contributor

@cacheflow cacheflow commented Oct 14, 2018

↪️ Pull Request

-Allow disabling of autoinstall via a environment variable named PARCEL_NO_AUTOINSTALL.

✔️ PR Todo

  • [x ] Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • [ x] Included links to related issues/PRs

Feature Request: #2081

jlfwong and others added 30 commits March 27, 2018 21:10
* handle empty config && asset files

* remove bad empty asset solution
3846masa and others added 8 commits October 6, 2018 16:53
<!---
Thanks for filing a pull request 😄 ! Before you submit, please read the following:

Search open/closed issues before submitting since someone might have pushed the same thing before!
-->

# ↪️ Pull Request

<!---
Provide a general summary of the pull request here
Please look for any issues that this PR resolves and tag them in the PR.
-->

Changed the logic to determine which package manager to use during auto-installation.

The only situation Parcel should use yarn is when it satisfies
- There is `yarn` command available
AND
- There is `yarn.lock` file
AND
- There is NOT `package-lock.json`

Otherwise, it uses npm.

Fixes #2117 

## 🚨 Test instructions

<!-- In case it is impossible (or too hard) to reliably test this feature/fix with unit tests, please provide test instructions! -->

Setup a testing directory with different setups (package-lock existence, yarn.lock existence, yarn command existence).
The only situation Parcel should use yarn is when
- There is `yarn` command available
AND
- There is `yarn.lock` file
AND
- There is NOT `package-lock.json`

## ✔️ PR Todo

~~- [ ] Added/updated unit tests for this change~~
- [x] Filled out test instructions (In case there aren't any unit tests)
- [x] Included links to related issues/PRs

<!--
Love parcel? Please consider supporting our collective:
👉  https://opencollective.com/parcel/donate
-->

## Side note

There is a situation where there are both package-lock.json and yarn.lock. Having npm as default package manager means that it should use npm in this situation.

That's why there is only one situation where Parcel should use yarn.
Very small change, but I noticed how these were calling `this.dest` directly over using the Packager's methods.
@mischnic
Copy link
Member

Maybe this is a better place for disabling autoinstall:

parcel/src/Bundler.js

Lines 137 to 140 in e86ddf4

autoinstall:
typeof options.autoinstall === 'boolean'
? options.autoinstall
: !isProduction,

@cacheflow
Copy link
Contributor Author

cacheflow commented Oct 14, 2018

Agreed @mischnic. Let me do the checking there.

-Allows disabling autoinstall via a environment variable named PARCEL_NO_AUTOINSTALL.
-Closes 2081.
DeMoorJasper
DeMoorJasper previously approved these changes Oct 15, 2018
@devongovett
Copy link
Member

Hmm why do we need an environment variable and not just use the cli flag that already exists?

@mischnic
Copy link
Member

I don't want to add the --no-autoinstall flag to every project of mine and having this flag set implicitly (so not specified in the package.json) doesn't change the build output.

@cacheflow
Copy link
Contributor Author

cacheflow commented Oct 15, 2018

I'd like the same in my projects @devongovett. 😄

src/Bundler.js Outdated
@@ -135,7 +135,8 @@ class Bundler extends EventEmitter {
detailedReport: options.detailedReport || false,
global: options.global,
autoinstall:
typeof options.autoinstall === 'boolean'
typeof options.autoinstall === 'boolean' &&
process.env.PARCEL_NO_AUTOINSTALL !== 'true'
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better if the options were positive instead of negative, so PARCEL_AUTOINSTALL=false should disable it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my original thought. Brb updating.

@devongovett
Copy link
Member

Due to move to monorepo which rewrote the git history, this PR was automatically closed. Please open a new PR against master with your changes. You can do a diff against the 1.x branch to see them. Sorry about that!

cacheflow added a commit to cacheflow/parcel that referenced this pull request Oct 16, 2018
cacheflow added a commit to cacheflow/parcel that referenced this pull request Oct 16, 2018
cacheflow added a commit to cacheflow/parcel that referenced this pull request Oct 18, 2018
cacheflow added a commit to cacheflow/parcel that referenced this pull request Oct 18, 2018
cacheflow added a commit to cacheflow/parcel that referenced this pull request Oct 23, 2018
cacheflow added a commit to cacheflow/parcel that referenced this pull request Oct 23, 2018
cacheflow added a commit to cacheflow/parcel that referenced this pull request Oct 30, 2018
devongovett pushed a commit to cacheflow/parcel that referenced this pull request Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.