-
Notifications
You must be signed in to change notification settings - Fork 58
Conversation
@Andarist |
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.
Looks good to me but you've accidently negated the entires.length check and you should really revert the deletion of the yarn.lock file, see the explanation here:
https://yarnpkg.com/lang/en/docs/yarn-lock/
Check into source control
All yarn.lock files should be checked into source control (e.g. git or mercurial). This allows Yarn to install the same exact dependency tree across all machines, whether it be your coworker’s laptop or a CI server.
If you use npm then you can simply ignore the file.
src/index.js
Outdated
|
||
// No aliases? | ||
if (!entries || entries.length === 0) { | ||
if (!entries.length === 0) { |
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.
You've accidently negated the entires length === 0 here, no? As such it should never work if its configurated correctly and the opposite way around 😄
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, my intention was to remove the first part of this OR, but I've accidentally missed this !
@thiscantbeserious master has currently 2 lock files - yarn.lock and package-lock.json. Only one of them should be kept and looking at main Rollup repository I've decided to keep package-lock.json as it seems to be the preferred one here. |
@thiscantbeserious I agree with @Andarist , |
@jacekk well the yarn.lock exists for a good reason and is a significant part of yarn's architecture - it does bring the advantage that, for example, the CI / Travis always uses 100% exactly the same dependencies (and sub-dependencies) that you used to build the project locally with (and the other way around). Going further along, by removing it you basically lose control over what dependencies you exactly used to build your package locally with the commit you made. Now that might be no issue for rolling release projects but it does lead to significant issues when you want to roll back to a previous version, for example since a few dependencies broke. If you have no history to keep track off - you can't easily roll back. You'll have to search the needle in the haystack - on the other hand if there are any issues with the yarn.lock in its current form there's nothing stopping you from doing a simple 'rm -f yarn.lock'. locally - on your own machine to create a new one that you can commit. If you really want to get rid of yarn.lock and its downsides (there are a few inconvieniences like fixed mirrors e.g. when you work behind a reverse proxy) you should also remove the package-lock.json, add both to gitignore and (!) make sure that your package.json only contains fixed versions. That means you'd get rid of all semvers (the most prominent one being ^). However you can't control sub-dependencies that way. So that's why I'd stick with the yarn.lock file. I'd never do that with a PR not directed towards messing with version management, that should really go into a seperate one tought since the bugs introduced could be significant (especially since npm is a dependency mess). |
@thiscantbeserious Tottaly agree with you about the ticket scope - any lock file should not be deleted within this very PR. Re lock file; I am aware of all pros and cons of yarn.lock file. The case is that The problem is that devs update NPM lock file and Travis uses |
@thiscantbeserious this is all true, locks are often important (not that important for libraries - especially small ones - in my opinion, but that's a separate discussion). The point here is that this repository had 2 lock files separate lock files and it should stick to only one of them. My intention is not to remove a lock file altogether - but rather just settle on a single one because having two doesn't make any sense.
That's a fair concern - I can move this to a separate PR. |
@jacekk actually based on - https://docs.travis-ci.com/user/languages/javascript-with-nodejs/#dependency-management - it's not even obvious what's the default, which one takes precedence on Travis. A table at the top suggests that it's npm - but builds on master are using yarn. It only kinda proves to me that we shouldn't keep 2 locks here because it ain't obvious which tool should be both used by developers and also which one a CI is actually using. EDIT:// moved those changes to separate PR - #62 |
Actually, this lines suggests that --> |
There is also (which comes first):
It seems though that yarn takes precedence - but this is not explicitly mentioned in the docs, as far as I can see. |
ping @shellscape @lukastaegert 👍 |
re: |
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.
One small change I'd like to see in the README, but if you disagree that's cool, Prettier will catch it when we move the plugin to the monorepo.
@shellscape adjusted |
I'd like to get one more maintainer to put eyes on this before we pull the trigger and merge. If that doesn't happen by tomorrow we'll forge ahead. |
|
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.
As stated, I would either throw an error if there are no entries
or fall back to the original syntax in that case.
This sort of fixes #58 .
It ain't fully backward-compatible with v1 but it brings back support for "simpler" object-based syntax for aliases. Why it isn't fully compatible? Because in v1 we would pass in alias-object right into the plugin, this PR requires it to be passed as
entries
option. I believe it's better to leave it like this (without introducing full backward compatibility) because it's less confusing around using other options (thanentries
) - v1 had this quirky behavior or allowing an alias map UNLESS it hadresolve
key (then you had to nest your alias map).