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

fix: loosen peerDependencies to document the oldest compatible package #506

Closed
wants to merge 1 commit into from

Conversation

jgravois
Copy link
Contributor

@jgravois jgravois commented Apr 9, 2019

tl:dr

it has been an error on my part to sync the peerDependencies manually with the devDependencies that lerna bumps automatically. we should be advertising the lowest compatible version, not the most recent.


@jPurush shared a thread with me the other day that made me realize:

  • lerna is making a conscious choice not to bump the peerDependencies for each package
  • if it is necessary to bump a peerDependency to exclude a previously compatible version, its likely that a breaking change is being introduced.

for example, we should have tagged v2.0.0 when we landed #400 because we introduced a dependency on a new cleanUrl() method inside the request package in existing functions in other packages.

folks even reported uncaught errors to me when cleanUrl() wasn't found in their own projects that were resolved by upgrading request. i knew enough about what was going on to recommend the fix to people but not enough to realize that we'd broken our contract.

getting in here and trying to figure out exactly what the minimum requirements were for our packages actually made the relationships more clear to me.

besides that, going forward we'll benefit from the lack of rote manual version bumping.

reference:

AFFECTS PACKAGES:
@esri/arcgis-rest-auth
@esri/arcgis-rest-feature-service-admin
@esri/arcgis-rest-feature-service
@esri/arcgis-rest-geocoder
@esri/arcgis-rest-groups
@esri/arcgis-rest-items
@esri/arcgis-rest-routing
@esri/arcgis-rest-sharing
@esri/arcgis-rest-users

AFFECTS PACKAGES:
@esri/arcgis-rest-auth
@esri/arcgis-rest-feature-service-admin
@esri/arcgis-rest-feature-service
@esri/arcgis-rest-geocoder
@esri/arcgis-rest-groups
@esri/arcgis-rest-items
@esri/arcgis-rest-routing
@esri/arcgis-rest-sharing
@esri/arcgis-rest-users
@jgravois jgravois requested a review from dbouwman April 9, 2019 23:40
@codecov
Copy link

codecov bot commented Apr 9, 2019

Codecov Report

Merging #506 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #506   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          95     95           
  Lines        1221   1221           
  Branches      227    227           
=====================================
  Hits         1221   1221

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6c40ef...84eb093. Read the comment docs.

@noahmulfinger
Copy link
Contributor

@jgravois Just thinking out loud, but I wonder if there's a way we can test packages with both the latest and oldest compatible version of their dependent packages, so stuff like cleanUrl doesn't slip through the cracks.

@patrickarlt patrickarlt mentioned this pull request Apr 11, 2019
35 tasks
@jgravois
Copy link
Contributor Author

i'm still glad i did the research, but this PR is irrelevant now.

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.

2 participants