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

Revert "use npm for auto-install by default" #2125

Closed
wants to merge 706 commits into from

Conversation

devongovett
Copy link
Member

Reverts #2119

Jasper De Moor and others added 30 commits March 27, 2018 20:16
* handle empty config && asset files

* remove bad empty asset solution
devongovett and others added 24 commits September 25, 2018 08:49
---
name: Add test to fs.readFileSync in TypeScript files
---
```
parcel-bundler version 1.9.7
```

## 💥 Problem

Inlining file in browser mode doesn't work in TypeScript files when `fs` is imported with star import or default import. The only way to use `readFileSync` is:
```typescript
import { readFileSync } from 'fs';
const raw = readFileSync(__dirname + '/raw.tsx', 'utf-8');
```

### Reproduction: 
Repo: https://github.com/hasparus/parcel-readfilesync-typescript-repro
ghPages: https://hasparus.github.io/parcel-readfilesync-typescript-repro/
CodeSandbox: https://codesandbox.io/s/github/hasparus/parcel-readfilesync-typescript-repro/tree/master/

Fun Fact: All ways to import `fs` work on CodeSandbox (parcel template).

## ↪️ Pull Request
Adds test to `fs.readFileSync` in TypeScript files and thus provides better workaround for #1736.

## ✔️ PR Todo
- [X] Link in #1736.
- [X] Post reproduction to CodeSandbox.
- [ ] Document how to use `readFileSync` in TypeScript?
Was running `parcel build` and the auto installer started running, I went to disable it and found that the option was not available here
# ↪️ Pull Request

Do not load existing sourcemaps if sourcemaps are disabled
---
name: 🙋 Fix HMR for Pug assets
---

<!---
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
Does this address an existing issue?
-->
#2090

## 💻 Examples

<!-- Examples help us understand the requested feature better -->

## ✔️ PR Todo

- [ ] Added/updated unit tests for this change
- [ ] Filled out test instructions
- [x] Included links to related issues/PRs
<!-- Love parcel? Please consider supporting our collective:
👉  https://opencollective.com/parcel/donate -->
<!---
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.
This reverts commit a0df886.
This reverts commit 1b808ec.
@Hoishin
Copy link

Hoishin commented Oct 12, 2018

@devongovett The problem is the case of having no lock file at all. Yarn enforces you to make yarn.lock even if you gitignore, first time you use yarn the file is made. It is a bit harder to generate no lock file with yarn.
On the other hand, it is entirely possible a bit easier to have no lock file using npm, just setting lock file to off in .npmrc.

(copy of #2119 (comment))

(edit)
I think the word "default" was misunderstood. It is a default in terms of using npm when there is no yarn.lock or package-lock.json.

(edit2)
Actually you can pass --no-lock-file to yarn install too... I think it is still valid opinion since you can have no lock file as project in npm, but not in yarn. For example I couldn't find a way to add a package with yarn without making yarn.lock.

@DeMoorJasper
Copy link
Member

Related issue #2117

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.

None yet