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

feat: add options with escapeSpecialCharacters #65

Merged
merged 2 commits into from Jul 30, 2023

Conversation

JoshuaKGoldberg
Copy link
Collaborator

Fixes #26. Fixes #63.

As described in #63, this introduces the concept of options with single option, escapeSpecialCharacters. The option defaults to:

  • true when called for a template literal's string tag
  • false when called as a function
// "\$hello!"
dedent.options({ escapeSpecialCharacters: false })`
  $hello!
`;

I'd played with allowing passing it in as a first argument instead of a string or template literal strings array, but that got complex. I suppose we can do that as a followup if people really want.

cc @G-Rath @sirian - what do you think?

@G-Rath
Copy link
Contributor

G-Rath commented Jul 27, 2023

I like it! though I wonder if something like withOptions or withSettings might be slightly better? (that might just be cause I've been doing a lot of Ruby lately...)

Also while I think this is overkill to do for now, if people really cared you could also do dedicated imports so folks could do:

import { dedentThatEscapesSpecialCharacters as dedent } from 'dedent';
import { dedentThatIgnoresSpecialCharacters as dedent } from 'dedent';

because while typically ugly names, there's no actual cost to those names from a package POV.

@Haroenv
Copy link
Collaborator

Haroenv commented Jul 28, 2023

I wonder if instead of an option, the real issue is that $ without a { after it doesn't need to be escaped, right?

@JoshuaKGoldberg
Copy link
Collaborator Author

something like withOptions or withSettings

Yeah, agreed, I like that. When I was coding it I kept going between .options and .with. withOptions it is!

overkill to do for now

Yeah if someone requests we can always go with that. Will wait 👍

$ without a { after it doesn't need to be escaped, right?

Oh, interesting. That's a good point. Filed #69. I'm going to go ahead and merge this option in because I do want to give folks the option to turn character escaping on or off as needed.

@SimenB
Copy link

SimenB commented Jul 31, 2023

This new option doesn't restore the old behaviour.

{
  "dependencies": {
    "dedent": "1.5.0",
    "dedent-1-3": "npm:dedent@1.3.0"
  }
}
const dedent = require('dedent');
const oldDedent = require('dedent-1-3');

const noEscapeDedent = dedent.withOptions({ escapeSpecialCharacters: false });

console.log(
  'old',
  oldDedent`
    const x = "snapshot";
    expect(1).toMatchSnapshot(\`my $\{x}\`);
  `,
);

console.log(
  'new',
  dedent`
    const x = "snapshot";
    expect(1).toMatchSnapshot(\`my $\{x}\`);
  `,
);

console.log(
  'new with option',
  noEscapeDedent`
    const x = "snapshot";
    expect(1).toMatchSnapshot(\`my $\{x}\`);
  `,
);
$ node file.js
old const x = "snapshot";
expect(1).toMatchSnapshot(`my $\{x}`);
new const x = "snapshot";
expect(1).toMatchSnapshot(`my ${x}`);
new with option const x = "snapshot";
expect(1).toMatchSnapshot(\`my $\{x}\`);

(I'd say the new behaviour is the correct one FWIW, but if an option is introduced to keep the change non-breaking, then it should restore the old behaviour, no?)

@JoshuaKGoldberg
Copy link
Collaborator Author

In the code example, escapeSpecialCharacters: false turns string escaping off always, regardless of call style. Setting it to true instead would enable the special character always, regardless of call style.

const yesEscapeDedent = dedent.withOptions({ escapeSpecialCharacters: true });

console.log(
  'yes yes yes',
  yesEscapeDedent`
    const x = "snapshot";
    expect(1).toMatchSnapshot(\`my $\{x}\`);
  `,
);
yes yes yes const x = "snapshot";
expect(1).toMatchSnapshot(`my ${x}`);

I'm not seeing why setting it to false is giving unexpected results here, sorry?

@SimenB
Copy link

SimenB commented Jul 31, 2023

Not unexpected per se, but different from before (1.3 and earlier). I agree new behaviour (with option true or false) is more correct, just surprising behavioural change in a minor release

@JoshuaKGoldberg
Copy link
Collaborator Author

Got it - makes sense. I was a little hesitant to release this as a minor rather than major for that reason. What swayed me was that I've been having a lot of discussions with folks recently about where semver applies or doesn't apply1. To me, this feels like it fits somewhere in-between "MAJOR version when you make incompatible API changes"2 and "MINOR version when you add functionality in a backward compatible manner"2. In my mind I landed on it being a MINOR because the changes to traditional users only impact nuances of character escaping, not the public API shape.

Footnotes

  1. E.g. in the TypeScript realm, https://www.learningtypescript.com/articles/why-typescript-doesnt-follow-strict-semantic-versioning. Admittedly a language's type system is not quite what we're talking about here. But it has been prompting me to think more critically about how much surface area we should cover as part of the public API, and how small a behavior change needs to be before we consider it non-breaking...

  2. https://semver.org/#summary. Quoting here for public visibility (I'm pretty sure I've seen you discuss these points SimenB and that you know the semver standard already 😄) 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants