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

Add support for outdenting string arguments. #8

Merged
merged 14 commits into from
Mar 17, 2018

Conversation

treshugart
Copy link
Contributor

@treshugart treshugart commented Mar 12, 2018

I'm not sure what your stance is on this but I thought I'd raise a PR just in case, as opposed to just raising an issue.

Use case

My use case for this is that we're using outdent to outdent code blocks for our Code component that highlights the input text:

<Code
  text={`
    Needs
      outdenting
  `}
/>

We could use it as part of that template literal, but what we want is for outdenting to automatically happen on the template strings in the component. In order to do this, we need to be able to format strings. Our current solution is to trick outdent by providing it an array with the raw property like so:

// @flow

type LitArray = Array<any> & {
  raw: Array<string>,
};

// This converts a string to the shape of a parsed literal that the outdent
// lit function uses.
export function strToLit(str: string): LitArray {
  const strAsLitArr: any = [str];
  Object.defineProperty(strAsLitArr, 'raw', {
    configurable: true,
    value: [str],
  });
  return strAsLitArr;
}

However, it feels like providing this behaviour as part of outdent itself is a far better idea.

Testing

I've tested locally using npm test and everything passes. However, we're getting the following error in CI:

ERROR: test/spec/spec.ts[1, 24]: Module 'chai' is not listed as dependency in package.json

Are you aware of this? If not, I can investigate and fix.

@cspotcode
Copy link
Owner

Thanks for the PR!

I want outdent to intentionally avoid touching the contents of interpolated values. So it'll remove indentation from the template literal, as it statically appears in source code, but never touch the contents of a value (even if that value is a string with newlines and whitespace). Buy that doesn't affect your use case; you're not even invoking outdent as a template tag; you just want to give it a string.

I'm also paranoid about overloading a single function with too many signatures. We already have two: one accepting a template literal array, the other accepting an options object. I can't think of a way this would break in the future, but you never know...

How about having outdent.string(stringValue)? It'll do exactly what you want: strip indentation from a string.

Sorry if this response is ignoring details you've already addressed in your PR; I'm not at my computer but I'll read the code later.

@treshugart
Copy link
Contributor Author

treshugart commented Mar 12, 2018

I'm also paranoid about overloading a single function with too many signatures.

I felt the same way but wanted to err on the side of convention here.

How about having outdent.string(stringValue)? It'll do exactly what you want: strip indentation from a string.

My original idea was along these lines. I'll happily update. The only thing would be choosing a good name. I'm ambivalent about outdent.string for a couple reasons:

  1. It feels like a separate export.
  2. string is reserved in typed langs like TS (if making it an export).

How would you feel about a named export, but called something like outdentString? If you're pretty set on outdent.string, then I'm totes fine with that, but thought it was worth bikeshedding since it'll be the public API :)

@cspotcode
Copy link
Owner

cspotcode commented Mar 12, 2018 via email

@treshugart
Copy link
Contributor Author

Re options, that's a fairly compelling reason, so your idea of outdent.string is far more palatable. This is especially since the API is unlikely to be growing any more than adding string. I'll update it to outdent.string to see how it looks. It's easy to convert to a named export if we feel that's better overall.

Thanks for the feedback!

@cspotcode
Copy link
Owner

Sounds good, looking forward to it. Thank you for pointing out the CI failures; I pushed a fix to master.

src/index.ts Outdated
@@ -133,6 +133,10 @@ function createInstance(options: Options): Outdent {
}
}

(outdent as any).string = (str: string): string => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will currently be breaking the build. I have absolutely no idea how to type this in TS. Halp.

Copy link
Contributor Author

@treshugart treshugart Mar 13, 2018

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

No worries, I'll make a commit to fix the types.

type Fn = () => string;
type Test = Fn & {
  method: Fn
};

That actually works in TypeScript; it was just balking at augmenting a function without first declaring the extended interface. (outdent as Outdent).string = does the trick.

One of these days I'll learn some Flow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, that would explain my confusion then. So close!

@cspotcode
Copy link
Owner

@treshugart I pushed some commits, and now all tests are passing. What do you think? If everything looks good to you I'll publish a new version.

@treshugart
Copy link
Contributor Author

Yeah, LGTM and thanks again :)

@cspotcode cspotcode merged commit e7925ee into cspotcode:master Mar 17, 2018
@cspotcode
Copy link
Owner

@treshugart I just published 0.5.0 to npm. Let me know if you encounter any issues.

@treshugart treshugart deleted the outdent-string branch March 19, 2018 10:50
@treshugart
Copy link
Contributor Author

Thanks @cspotcode, will do!

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