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

Update dependencies that were effectively downgraded & remove some unused dependencies #969

Merged
merged 2 commits into from
Mar 1, 2018

Conversation

realityking
Copy link
Contributor

@realityking realityking commented Feb 21, 2018

πŸš€ Why this change?

In the move to fixed dependency versions (f0f200b) some dependencies were effectively downgraded as there was a newer, in-range, version at the time.

While looking at the dependencies, I also found some unused dev dependencies:

  • coffee-coverage isn't used anymore after decaffeinating

Many more things that are outdated but this seem like obvious wins πŸ™‚

πŸ“ Related issues and Pull Requests

βœ… What didn't I forget?

  • To write docs
  • To write tests
  • To put Conventional Changelog prefixes in front of all my commits and run npm run lint

@michalholasek michalholasek mentioned this pull request Feb 21, 2018
3 tasks
@honzajavorek
Copy link
Contributor

honzajavorek commented Feb 22, 2018

Thanks! This is definitely a good idea. It will take me some time to check licensing etc. of the packages, will track the progress here:

@honzajavorek
Copy link
Contributor

@realityking I think once the eslint-config-airbnb-base is sorted, we're good to go, but without winston for now. With regard to #765 and the long-term intention to replace winston with something like debug I'm honestly a bit lazy to proceed with the upgrade internally right now.

@realityking realityking force-pushed the dependencies branch 2 times, most recently from 0ec5369 to 0ea97f6 Compare February 22, 2018 17:56
@realityking
Copy link
Contributor Author

Thanks @honzajavorek! I pushed a new version without the bump for winston.

As for eslint-config-airbnb-base, that's already a dependency through eslint-config-airbnb but I'm happy to split this into another PR.

@honzajavorek
Copy link
Contributor

I'm happy to split this into another PR

Let's wait just a little bit, my hunch would be this one could get resolved in a timely manner. It's Airbnb, they could care about licensing πŸ˜„

@ljharb
Copy link

ljharb commented Feb 22, 2018

Note that the "license" field in package.json, which is SPDX-compliant, is more than sufficient, the actual LICENSE file is not required. (that said, I've merged airbnb/javascript#1746)

@honzajavorek
Copy link
Contributor

@ljharb Thanks! πŸ‘ I answered to your comment under airbnb/javascript#1746.

It's not used anymore.
@realityking
Copy link
Contributor Author

Based on the above discussion I removed the eslint-config-airbnb since I assume you'll want to update to a version that includes the License file.

I'll open a separate PR to track that.

@ljharb
Copy link

ljharb commented Feb 24, 2018

With or without the license file, the dependency is MIT licensed; there should be no need to remove it.

@honzajavorek
Copy link
Contributor

@ljharb I'm sorry, but it seems the interpretation may differ from lawyer to lawyer. In that case, I may have the bad luck of having stricter lawyers behind my back. They require me to have only dependencies with copyright notice including a copyright holder and with a full license text.

Vast majority of projects I've seen on npm or elsewhere just include the full license text, even the smallest ones. (Probably thanks to GitHub, which puts you the license file into the repo from the very beginning.) Observing that, I have no particular reason to fight our lawyers and persuade them that three letters are enough. I've seen myself projects using MIT licenses, but with their license wording being slightly different. I've seen MIT in package.json and a completely different license text or dual licensing in the LICENSE file. It can be quite a mess. So I suppose this is mostly defensiveness and just being 100% sure what were the intentions of the project author.

I'm happy to go and contribute licenses anywhere where they're missing or file issues for discrepancies, because licenses are important, it's the main glue of the FOSS.

That said, we started to properly care about licenses just about recently and any dependencies we're already using previously are a priori pardoned. So when talking about removing eslint-config-airbnb from this PR, it's just about not upgrading its version yet, not removing the dependency from the project πŸ™‚ (cc @realityking )

@honzajavorek
Copy link
Contributor

@realityking btw I'm sorry for the failing Windows builds, they got very flaky recently. I plan to focus on fixing them, but until that time I'll keep retrying the builds for you πŸ˜•

@realityking
Copy link
Contributor Author

@honzajavorek Looks like this all passes now :)

Copy link
Contributor

@honzajavorek honzajavorek left a comment

Choose a reason for hiding this comment

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

πŸ’ƒ

@honzajavorek honzajavorek merged commit e46f225 into apiaryio:master Mar 1, 2018
@realityking realityking deleted the dependencies branch March 1, 2018 09:40
@realityking
Copy link
Contributor Author

@honzajavorek Would it be possible to release a new minor version that includes this PR? πŸ™‚

@honzajavorek
Copy link
Contributor

Ha, I haven't noticed it's classified as chore:. If it was feat: or fix:, it gets autoreleased. Hmm...

@realityking
Copy link
Contributor Author

Ah, good to know. Now I wish I had made #979 a fix: - I was debating that πŸ˜†

@realityking
Copy link
Contributor Author

I had another PR I wanted to make this week - let me do that now and I'll put it in as a fix :)

@honzajavorek
Copy link
Contributor

Yeah, I think fix fits better. Maybe I can find time to do upgrades on Dredd Transactions and to release a new Dredd with those propagated.

honzajavorek added a commit that referenced this pull request Mar 5, 2018
Because #969 was mistakenly classified as `chore`.
@honzajavorek
Copy link
Contributor

honzajavorek commented Mar 5, 2018

Decided not to block on DT or other PRs, this resolves the issue: #980

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

3 participants