feat: use fallback if prettier not found #11400
Conversation
|
Hi @connorjclark! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
| @@ -73,6 +73,36 @@ expect(a).toMatchInlineSnapshot(\`[1, 2]\`); | |||
| ); | |||
| }); | |||
|
|
|||
| test('saveInlineSnapshots() with bad prettier path leaves formatting outside of snapshots alone', () => { | |||
There was a problem hiding this comment.
This is literally a copy/paste of the previous test case, but with a phony install path.
|
I think this approach is fine since the prettier path you're dealing with is already the default |
| try { | ||
| // @ts-expect-error requireOutside Babel transform | ||
| prettier = requireOutside(prettierPath) as Prettier; | ||
| } catch (_) { |
There was a problem hiding this comment.
| } catch (_) { | |
| } catch { |
There was a problem hiding this comment.
Haha, TIL you can use no arguments. Didn't realize you just have to leave off the parentheses. thx.
|
Sorry for delay–been driving cross-country last couple days. updated the PR. I am also hunting down the right person to email the FB CLA address. |
Codecov Report
@@ Coverage Diff @@
## master #11400 +/- ##
==========================================
+ Coverage 64.20% 64.22% +0.02%
==========================================
Files 309 309
Lines 13525 13527 +2
Branches 3294 3294
==========================================
+ Hits 8684 8688 +4
+ Misses 4126 4125 -1
+ Partials 715 714 -1
Continue to review full report at Codecov.
|
|
@connorjclark hey, any news? 🙂 We're just about ready to release v27, and this is in the milestone. As it's not breaking in and of itself we can probably land it later, tho 👍 |
|
We need this in for 27, without it inline snapshots without Prettier don't work as advertised |
|
😞 |
Haven't heard back from cla@fb.com, I'll send another email. |
|
@JoelMarcey do you know why the CLA might be stuck? |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
Yay! Thanks! |
| try { | ||
| // @ts-expect-error requireOutside Babel transform | ||
| prettier = requireOutside(prettierPath) as Prettier; | ||
| } catch { |
There was a problem hiding this comment.
thinking about it, we should probably just swallow MODULE_NOT_FOUND and rethrow other errors. Meh 🤷
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
ref #11386
This allows the require for prettier to fail, without bubbling up as an error.
An alternative to this approach would be to change
Defaults.tsto attempt settingprettiertorequire.resolve(prettier)(or something), but I am not familiar enough with the codebase to know if that'd be OK. The current approach works, but may hide an accidental misconfiguration if a user manually setsprettierPath.