Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

Add support for object syntax #61

Merged
merged 1 commit into from Oct 18, 2019
Merged

Conversation

Andarist
Copy link
Member

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 (than entries) - v1 had this quirky behavior or allowing an alias map UNLESS it had resolve key (then you had to nest your alias map).

@jacekk
Copy link

jacekk commented Oct 13, 2019

@Andarist yarn.lock has been removed. I understand why, but I would additionally gitignore it. Just in case someone else would (even accidentally) commit it back.

Copy link
Contributor

@thiscantbeserious thiscantbeserious left a 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 Show resolved Hide resolved
src/index.js Outdated

// No aliases?
if (!entries || entries.length === 0) {
if (!entries.length === 0) {
Copy link
Contributor

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 😄

Copy link
Member Author

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 !

@Andarist
Copy link
Member Author

you should really revert the deletion of the yarn.lock file, see the explanation here:

@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.

@jacekk
Copy link

jacekk commented Oct 14, 2019

@thiscantbeserious I agree with @Andarist , yarn.lock should be removed and gitignored - as advised in that docs, you had referenced. SSOT is kinda crucial when it comes to reliable builds.

@thiscantbeserious
Copy link
Contributor

thiscantbeserious commented Oct 14, 2019

@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).

@jacekk
Copy link

jacekk commented Oct 14, 2019

@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 package-lock.json seems to be more up to date and more reliable when it comes to dependencies (and sub-dependencies), thus I would do the same what @Andarist did.

The problem is that devs update NPM lock file and Travis uses yarn.lock file - https://docs.travis-ci.com/user/languages/javascript-with-nodejs/#yarn-is-supported . Maybe that is OK with you, but I personally keep yarn.lock within my own repos and gitignore package-lock.json file. Just to not confuse anyone, and still use all its advantages.

@Andarist
Copy link
Member Author

@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.

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).

That's a fair concern - I can move this to a separate PR.

@Andarist
Copy link
Member Author

Andarist commented Oct 14, 2019

@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

@jacekk
Copy link

jacekk commented Oct 14, 2019

@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.

Actually, this lines suggests that --> If both package.json and yarn.lock are present in the current directory, we run the following command instead of npm install: :)

@Andarist
Copy link
Member Author

There is also (which comes first):

If package-lock.json or npm-shrinkwrap.json exists and your npm version supports it, Travis CI will use npm ci instead of npm install.

It seems though that yarn takes precedence - but this is not explicitly mentioned in the docs, as far as I can see.

@thiscantbeserious
Copy link
Contributor

ping @shellscape @lukastaegert 👍

@shellscape
Copy link
Contributor

re: yarn.lock - we'll be moving this plugin to the plugins repo as soon as we're able to get through the issues and open PRs (that's the first step), and the plugins monorepo leverages pnpm, so yarn.lock will be nuked and yarn will not be supported in the repo. Sorry yarn users, but we're trying out something different.

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@shellscape shellscape left a 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.

@Andarist
Copy link
Member Author

@shellscape adjusted

@shellscape
Copy link
Contributor

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.

@lukastaegert
Copy link
Member

lukastaegert commented Oct 17, 2019

I can check tomorrow Actually I can have a quick look right away

Copy link
Member

@lukastaegert lukastaegert left a 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.

src/index.js Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make v2 compatible with v1
5 participants