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

Make prettier a peerDependency #1

Merged
merged 1 commit into from
Jan 28, 2017

Conversation

lydell
Copy link
Member

@lydell lydell commented Jan 28, 2017

This allows the user to choose their version.

This allows the user to choose their version.
@not-an-aardvark
Copy link
Member

Thanks, this looks good. However, I do have a few concerns about this:

  • I think it's a breaking change, because people who weren't installing prettier manually will now have to do so. (This is not really a problem, I just want to double-check.)
  • If prettier's API gets updated in a future major version (e.g. if it becomes prettier.formatText instead of prettier.format for whatever reason), this plugin won't be usable on both versions, because it depends on the current version of the prettier API, even though it's agnostic to the version of the prettier printer.

I still think this change might be worth it anyway, but do you agree with my assessment of these issues?

@lydell
Copy link
Member Author

lydell commented Jan 28, 2017

  • Yes, I agree that it is a breaking change.

  • Regarding prettier’s API:

    • prettier’s API is so simple that it is unlikely to change – there’s no reason to. What’s going to change much more is its output. So in reality, I don’t think there’s very much to worry about.
    • If/when prettier does change the API, this plugin could support several prettier versions through duck typing and/or change the peerDependency version constraints and release new (breaking if needed) versions as needed.

So, yes, I think I agree with your assessment.

I guess it makes sense to allow >=0.11.0 (with no upper bound) as long as prettier is 0.x, since we can’t know what version might bring a breaking change. Or perhaps we should add <1.0.0? When prettier has become 1.x, I guess we could set the upper bound to <2.0.0 as a safeguard against potential breaking changes in the future.

@not-an-aardvark
Copy link
Member

I suppose if prettier's API does change, it will only break this plugin for new installs, because people who previously installed an older version will still be getting it. I think that's probably fine -- if it does happen then I can make a patch release for the plugin. (I mainly want to make sure that the plugin can't unexpectedly stop working without user intervention.)

Copy link
Member

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@not-an-aardvark not-an-aardvark merged commit d8a8992 into prettier:master Jan 28, 2017
@lydell lydell deleted the peer-dependency branch January 28, 2017 20:05
},
"devDependencies": {
"eslint": "^3.14.1",
"eslint-config-not-an-aardvark": "^2.0.0",
"eslint-plugin-eslint-plugin": "^0.2.1",
"eslint-plugin-node": "^3.0.5",
"mocha": "^3.1.2"
"mocha": "^3.1.2",
"prettier": ">= 0.11.0"
Copy link
Member Author

@lydell lydell Jan 28, 2017

Choose a reason for hiding this comment

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

The version range does not technically have to be >= 0.11.0 here; ^0.11.0 could work as well. I thought it might be a good idea to use the same range as the peerDependency, though, to make sure that the tests pass when using the latest version allowed by the peerDependency. When I made the PR, the latest version of prettier on npm was 0.11.0. Now, when the PR was merged, 0.13.1 has been released, with slightly different output. I didn’t realize that this repo uses prettier for linting itself, which caused Travis to fail on the merge commit. Thoughts?

  • Change the devDependency version constraint?
  • Remove prettier from the eslint rules of this module?
  • Just live with it, and re-format the files every now and then?

Copy link
Member

Choose a reason for hiding this comment

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

I suppose I'll pin the version of prettier that this module uses to lint itself so that the build doesn't unexpectedly break.

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