Closed ntkoopman closed 2 years ago
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:
literalOf
wrapperarrayOf
wrapperIs 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 :thinking: ).
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.
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
.
@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!
Released as 4.10.0
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 thinkliteralOf
should just support strings as well (and basically everything else).This PR doesn't touch
deepGenerate
, which means thatcode`const a = ${map}`
will work correctly butcode`const a = ${[map]}`
doesn't. The issue is that at the pointdeepGenerate
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, whiledeepGenerate
only generates code at the end:An an actual issue: if you put a
MaybeOutput
inside aLiteral
it will never show up.