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

Extend support for literals #23

Merged
merged 3 commits into from
Jan 8, 2022
Merged

Conversation

ntkoopman
Copy link
Contributor

I wanted to add a string and thought it wasn't obvious on how to do it. Only while implementing this I found that I could have just used JSON.stringify, but I think literalOf should just support strings as well (and basically everything else).

This PR doesn't touch deepGenerate, which means that code`const a = ${map}` will work correctly but code`const a = ${[map]}` doesn't. The issue is that at the point deepGenerate is executed, all the aliasing has already happened so it's too late to construct a Literal. Fixing this would require quite a lot of refactoring.

Another difference is that Literal will create the literal at construction time, while deepGenerate only generates code at the end:

const a = { foo: 'bar'};
const b = code`
  const a = ${[literalOf(a)]};
  const b = ${[a]};
`;
a.foo = 'qux';
console.log(b.toString())
const a = { foo: "bar" };
const b = { foo: "qux" };

An an actual issue: if you put a MaybeOutput inside a Literal it will never show up.

@stephenh
Copy link
Owner

stephenh commented Jan 7, 2022

Nice! Thanks for digging into this.

Fwiw, the support for literals has grown organically and I'm not against re-thinking it...i.e. the current approach of:

  • String needs literalOf wrapper
  • Array needs arrayOf wrapper
  • Maps don't

Is not great. I think the reason is historical b/c early on I had a pattern of doing:

const chunks = Code[];
chunks.push(code`...`);
chunks.push(code`...`);
return code`.... ${chunks} ...`

And, in the above scenario, I wanted the Code[] to be output without the wrapping [ ] and interspersed ,.

So I originally did that. But then later I did actually want to output literal arrays, so here came arrayOf ... then later object literals, which was objectLiteral... most recently I wanted a map and for whatever reason decided that shouldn't need a wrapper (or maybe I just forgot about literalOf already existing 🤔 ).

Anyway, I'm wondering if we should just kill arrayOf and literalOf and have interpolation of non-Code data just always output their JSON.stringifiied-ish notion. So you could do probably the most intuitive thing of const foo = ${"string"} and you'd get const foo = "string" as the ouptut...

This would be a breaking change, but dunno, it'd be pretty easy to find the ~probably 20 or so places across ~10 or so projects I'm doing ${chunks} and change it to ${joinCode(chunks, "")}. So it's an easy fix.

I suppose we could also canonicalize the "chunks" pattern and make like a const chunks = new Chunks(), with chunks.push, chunks.push, and then ${chunks} still outputs the "not an array literal" version of things.

Literal will create the literal at construction time

That's interesting, and the mutation of placeholders isn't something I'd thought of.

We could probably keep that behavior by changing the function code impl around index.ts:13 to apply a (potentially now internal-only literalOf) to all placeholders instead of just maps/plain objects? (I.e. again I think maps/isPlainObject being handled automatically in the code function vs. the arrayOf / literalOf approach is from me implementing them ~a year or so plus apart.)

Wdyt? Do you want to try tackling this in this PR? I.e. deprecate arrayOf / literalOf, make ${array} output an array literal / ${...} always literalizes any non-Code placeholders? If that is getting too hairy for ts-poet's admittedly-written-as-things-went-along I can take a crack at it as well sometime soon-ish.

@ntkoopman
Copy link
Contributor Author

I had similar questions when looking at this. I didn't go that way because I would like being able to do something like

code`
  class Foo {
    ${needConstructor && code`constructor() {}`}
    ${methods.map(x => code`${x.name}(){ ... }`)}
  }` 

similar to how JSX works (ignore null/undefined/boolean as well). I also would expect code`class ${name}{ ... }` to output class Foo{ ... } instead of class "Foo"{ ... }. I'd rather require wrapping objects in literalOf as well, and throw an error on "plain" maps.

I'm also actually ok with the current behavior (well, besides ${map} and ${[map]} having different output) even though it's a bit inconsistent. You could remove the arrayOf method, since it's the same as passing an array to literalOf.

@stephenh
Copy link
Owner

stephenh commented Jan 8, 2022

@ntkoopman ah yeah codeclass ${name} is a great/obvious example of why we should keep the current behavior + JSX usages... thanks for the great sanity check / push back.

Thanks for the PR!

@stephenh stephenh merged commit 198fe5a into stephenh:master Jan 8, 2022
@stephenh
Copy link
Owner

stephenh commented Jan 8, 2022

Released as 4.10.0

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