Skip to content
This repository was archived by the owner on Apr 1, 2023. It is now read-only.

Dependency cleanup #122

Closed
wants to merge 7 commits into from
Closed

Dependency cleanup #122

wants to merge 7 commits into from

Conversation

cdfa
Copy link

@cdfa cdfa commented Jan 22, 2019

Fixes #120, #119 and #77

Changes:

  • Update dependencies
  • move prop-types dependency from example to lib. It's weird to put this as a peer dependency, because we only import it in the library and a user may choose to not use prop-types or something different.
  • remove react-dom dependency from library (still used in example). It doesn't look like this is actually used.
  • add eslint-import-resolver-node (doesn't result in build error, but eslint complains without it)
  • move all babel config to .babel.rc
  • Add all installed plugins to babel config

Notes

cdfa added 7 commits January 22, 2019 15:57
Changes:
* Update dependencies
* `babel-preset-stage0` is now deprecated. I think it's best to let users just add what they need themselves.
* warning about rollup postcss plugin using the onwrite hook: egoist/rollup-plugin-postcss#148
* move prop-types dependency from example to lib. It's weird to put this as a peer dependency, because we only import it in the library and a user may choose to not use prop-types or something different.
* the specific versions babel-eslint and eslint are required by the latest version of react-scripts.
* remove `react-dom` dependency from library (still used in example). It doesn't look like this is actually used.
* add eslint-import-resolver-node (doesn't result in build error, but eslint does give an error without it)
I merged the previous changes from my own repo where I had updated the dependencies from the 2.6.7 version of the dependencies, in which babel 6 was still used.
I removed this in the library I created with this starter, because I don't need it, but forgot to remove it from the commit on this repo.
I don't know why this didn't happen automatically
Using "external-helpers" plugin with rollup-plugin-babel is deprecated, as it now automatically deduplicates your Babel helpers.
This should have been done right when upgrading to babel 7
@cdfa
Copy link
Author

cdfa commented Jan 22, 2019

Something is broken with the testing. When i try to run them myself it eats up way to much ram and the travis ci log says it's finding eslint 4.18.2, which is not the one specified in the new, or old root package.json. I don't get the error myself and it's annoying me that npm looks for packages up the directory tree, preventing the root project to use a different npm version than react-scripts. That's just a flawed design from the start.

@paulmelnikow
Copy link

Hey! I've been meaning to dip my toe in to this library and joined some discussions but realized I wasn't following, and missed this before.

There's a lot happening in this PR which makes it a bit difficult to review. What do you think about picking out little pieces into new PRs and trying to get them merged?

I don't have merge access (though @transitive-bullshit started a convo about that #104 (comment)). However I'd be happy to review and 👍 pieces of this, and I think it would expedite this update.

@paulmelnikow
Copy link

  • move prop-types dependency from example to lib. It's weird to put this as a peer dependency, because we only import it in the library and a user may choose to not use prop-types or something different.

Agreed this is a good idea. When I use prop-types in my libraries, I make it a dependency. I think the counterargument is trying to avoid multiple versions of prop-types being installed, though I tend to trade off in favor of simplicity by making it a dep. It's not very big.

@paulmelnikow
Copy link

  • remove react-dom dependency from library (still used in example). It doesn't look like this is actually used.

👍

],
"dependencies": {
"prop-types": "^15.6.2"
}

Choose a reason for hiding this comment

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

Could you move this up before peerDependencies so it's more noticeable?

@paulmelnikow
Copy link

  • Add all installed plugins to babel config

This makes sense to me.

@cdfa
Copy link
Author

cdfa commented Feb 11, 2019

There's a lot happening in this PR which makes it a bit difficult to review. What do you think about picking out little pieces into new PRs and trying to get them merged?

Yeah, I realized there's too much happening when I submitted the pull request, but I was quite fed up with these "chores" so I didn't split it up. I'll move a few things from my "change log" in my initial comments to notes for a start.

Al least it's good to see you agree with most of the changes though.

Do you have any idea why the CI tests are failing? I'm not really willing to continue working on this if I don't know what I'm doing wrong there. Does everything work fine with you? Please let me know.

@transitive-bullshit transitive-bullshit mentioned this pull request Mar 5, 2019
4 tasks
@transitive-bullshit
Copy link
Owner

Thanks @cdfa.

I finally got around to updating CRL and releasing v3.0.0. Please feel free to follow-up with an updated PR and sorry for taking so long to get back to you.

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.

Missing estraverse dependency
3 participants