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

jsxBracketSameLine updated after deprecation #3

Closed
wants to merge 1 commit into from

Conversation

Serg4554
Copy link

PR for this issue to avoid the warning: #2

PR for this issue to avoid the warning: npetruzzelli#2
@alessio-libardi
Copy link

Hello @npetruzzelli can we merge this PR?

@npetruzzelli
Copy link
Owner

npetruzzelli commented Jan 19, 2022

Thank you for commenting @alessio-libardi - its odd that this is the first email notification I've received about this for this repo. I didn't get one for @Serg4554 or @rotkiw

I need to carefully consider how to handle versioning. Likely, I will be considering this a breaking change that requires a new major version (of the config, not of Prettier), since the config would no longer be compatible with Prettier versions 2.0.0 through 2.3.x, the result of the config would be different based on the Prettier version not the config version.

For now it is a warning on Prettier's side and nothing is broken, since they deprecated it, they were able to release it on a minor version.

I realize this may affect systems that may be strict enough to require zero errors and zero warnings of any kind, so I'll try to tackle this sooner rather than later.

@alessio-libardi
Copy link

alessio-libardi commented Jan 20, 2022

Thanks to you @npetruzzelli for taking the time to maintain this repo 🙌

By the way I'm not sure this should be a major, since updating prettier but not prettier-config-standard will cause for the large majority of users (the ones using ^ on their deps) the wrong formatting options to be applied. This especially apply on CI environment in which you use npm ci, this will get you the latest prettier without updating it because its on the same major but you won't get the update on prettier-config-standard because its a major update.

I'm not sure I explaned too well so I'll make you an example:

Given this package:

"prettier": "^2.3.1"
"prettier-config-standard": "^4.0.0"

Locally you could format using [email protected] because the last time you installed your modules and changed your package-lock was a couple months ago, meanwhile on your CI environment that runs everytime you push your code to the server the pipeline that checks your format fails because it install [email protected] and result in different formatting.

Let me know what you think 😄

@npetruzzelli
Copy link
Owner

I would like to apologize for the delay. It continues to be a very busy time both in and out of work for me.

My goal with this config is reliability and predictability, regardless of whether or not you are using exact versions or not and regardless of whether or not you are using a lock file. I look at config files a bit differently than modules or applications.

If you are using a lock file, it should be committed to your repo and your CI process would then honor it. Both npm and Yarn have command line arguments to prevent the creation of lock files if you aren't using them.

As long as someone uses a given major version of Prettier (2.x) and a major version of this config, then the results should be the same. I can't do anything for people using lower minor patches when new features are introduced in higher minor patches, but I can handle the reverse.

There is a reason that settings are deprecated before being removed: 1) give consumers time to respond; 2) honor the contract of semantic versioning.

If I remove the deprecated property and add its replacement in a minor or patch version, then users of lower versions of Prettier 2.x would revert to the default behavior instead of an explicitly defined behavior, and this would go against the spirit of the work done for issue #1 (and it is part of why it was broken up between multiple major versions of this config).


There is a work around available until I get to this. You can extend this config using the instructions described in the README and manually remove the prop giving the warning and replace it with the new one. When I upgrade this config, it will be a major version upgrade. This won't matter to anyone using a lock file, since it will be a manual step to upgrade regardless of whether it is a major, minor, or patch version. New installs with npm install --save-dev prettier-config-standard will get the latest version either way.

// `prettier.config.js` or `.prettierrc.js`
const prettierConfigStandard = require('prettier-config-standard')

const modifiedConfig = {
  ...prettierConfigStandard,
  bracketSameLine: true
}
delete modifiedConfig.jsxBracketSameLine

module.exports = modifiedConfig

This should prevent warnings since the final config that gets consumed will not have the deprecated property defined at all.

I have not (yet) checked whether or not deleting the deprecated property is needed or if simply defining the new property is enough.

@npetruzzelli
Copy link
Owner

For context, I never install Prettier globally. Both my command line tools and code editor plugins use versions of Prettier local to the project I am working on. I don't think it affects the above, but it might help convey the scenario this config was built to thrive in.

@Serg4554
Copy link
Author

Hi @npetruzzelli, I appreciate that you spend your time considering the versioning to not break any existing project using your library!

I agree with you about releasing a new major version, as minor versions are not supposed to break anything, but if you upgrade this library without upgrading prettier to the proper version, it will not be compatible at all and break the formatting of brackets. I expect prettier also releasing a major version once they stop supporting the old flag, but in my opinion, a deprecation warning is enough to release a new major version that will not be compatible with prettier < 2.4.0

So what if you define the dependency with prettier in package.json?

"dependencies": {
  "prettier": "^2.4.0"
}

It shouldn't affect the build size of any project (as both, prettier and this project should be devDependencies) and makes explicit the dependency with prettier and the minimum required version, so when someone upgrades prettier-config-standard to 5.x.x, it will also upgrade prettier to the next compatible minor version (as long as the caret ^ is also present in prettier).

WDYT? Feel free to correct me if I made any wrong assumptions 🙂

@rotkiw
Copy link

rotkiw commented Jan 31, 2022

Hello guys!

My thoughts:

  • Since 2.4.0 jsxBracketSameLine not working at all. It's a breaking change in a minor version.
  • Their way to install prettier with --save-exact is a lazy way to escape responsibility.
  • Please do not follow them and make a new major version with prettier >=2.4.0 in the peerDependencies. Use the README to make the situation clear.
  • You can also add peerDependencies to the current version:
"peerDependencies": {
  "prettier": "<2.4.0"
}

My current solution:

const { jsxBracketSameLine, ...prettierConfigStandard } = require('prettier-config-standard')

module.exports = {
  // apply Standard formatting
  ...prettierConfigStandard,

  // jsxBracketSameLine deprecated
  bracketSameLine: jsxBracketSameLine, // current settings: true
}

And of course, thank you @npetruzzelli for your great work!

@alessio-libardi
Copy link

Hello @npetruzzelli ,I know the versioning needs careful consideration, but do we really think versioning is more important than the feature the libs itself provides? Shouldn't we just bump it for good at this point?

Since the code is ready, doing semver philosophy around this it's not ideal, any way you will upgrade someone will still complain about the way we bumped the version and they'll eventually upgrade anyway if they need this config...there's just one thing we all have in common, we need this upgrade :)

What do you think? By the way if you need help mantaining the repo I can definitely try to help 🙌

@rotkiw
Copy link

rotkiw commented Feb 18, 2022 via email

@npetruzzelli
Copy link
Owner

It has been a very busy year in work and out. Once more I apologize for taking so long for simple responses.

I've finally got around to doing some local testing and some of what's being said here is making more sense as I get more context.

Generally, when you deprecate something, the functionality continues to work until until it is removed in a major patch version.

With Prettier 2.4, they did not just add a new option name in the form of bracketSameLine, but they disabled the deprecated rule completely. Prettier will still recognize it, so we receive the message:

[warn] jsxBracketSameLine is deprecated.

instead of

[warn] Ignored unknown option { jsxBracketSameLine: true }.

which we can probably expect in Prettier 3.x


At least for Prettier@^2.4.0 both jsxBracketSameLine and bracketSameLine can exist in the same config, but that still results in a warning.

I can't say I agree with the approach Prettier took here. If you ignore the addition of bracketSameLine, the disabling of jsxBracketSameLine is a breaking change in the world of semantic versioning and deserving of a new major version.

For individual configs, it isn't a big deal. Update your config and you are good to go (because the feature still exists, if under a different name).

For shareable configs, if you want make the claim that your major version will produce the same results across the same major version of the formatter, with minor versions only configuring new features and patch versions only fixing bugs, then you are put into a conflicting position.

  • spread the major version of the formatter across multiple major versions of your config so that @^X.Y.Z still works in a predictable way for consumers?
  • increment your minor version, adding only the new property name, but leave the old one. This will work for all of Prettier 2.x, but you still get the warning and still has the problem of lower versions of your config producing different results with Prettier 2.4+.
    • This may be an edge case that shouldn't normally be an issue, but if you've made the "produce the same results" commitment, then it is a problem you need to address as the config maintainer all the same.

@rotkiw 's workaround is great. Instead of relying on a hard coded work around value, it is flexible enough to update itself in the future, so long as the valid values for jsxBracketSameLine remain valid values for bracketSameLine and have the same results.


Because I've made the "produce consistent results" promise and Prettier 2.4.0 is a breaking change in that regard, then I have to abandon the hope of settling into a "one config major version per formatter major version" path.

STEP 1: Moving forward, I will first add a peer dependency that requires "prettier": ">= 2.0.0 < 2.4.0" to the current major version of prettier-config-standard (via a new minor version)

With this in place, installers like npm and Yarn can provide meaningful messages during installation. The user doesn't have to wait until they try to use Prettier to get Prettier's own warning, which does not include versioning information.

STEP 2: Afterwards, I will create a new major version that sets a peer dependency of "prettier": "^2.4.0" and it will remove jsxBracketSameLine and add bracketSameLine.

Reference:


@alessio-libardi - I appreciate the offer, but I haven't reached that point. If I were to consider that, then one blocker is I haven't put together any kind of "contributing" agreement/document just yet, and no one can fix that except me! While the majority of the community is fantastic and understand what open source contributions are, others can cause problems if something doesn't go the way they like. Until I can become more familiar with intricacies of the legal side of open source, my priority is to protect the code so that everyone can continue to use it!

I may be overly worried about this because I watched it bring an open source Java project to a standstill. It has been so long that I don't remember the details, I just remember it was disheartening to watch.

@npetruzzelli
Copy link
Owner

closed by #4

Thank you @Serg4554 and everyone who contributed to the discussion and waited patiently for the update.

@npetruzzelli
Copy link
Owner

npetruzzelli commented Feb 26, 2022

For anyone in the "I just need the update!" camp

It is now available on npm as version 5.0.0: https://www.npmjs.com/package/prettier-config-standard

@Serg4554 Serg4554 deleted the patch-1 branch February 26, 2022 19:09
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.

5 participants