Closed MichalBryxi closed 2 months ago
Ok, this PR is ready for review. It fixed the issues. I failed with every other avenue. It adds good chunk of test coverage. And cleans up few bits.
cc: @gitKrystan
Maybe it would be beneficial to use a placeholder character that is less common than -
?
Unless I'm completely missing the way this change works.
That's possible, with few notes:
${PLACEHOLDER}
character should not (famous last words) make it anywhere back to the user. It's used just to "take the same amount of byte space" as /
does.<template> ... </template>
with JS comment: {/* ... */}
and we must ensure that things inside that comment don't cause an issue in the layers that code.*/
).#
- Private fields$
- jQuery%
- modulo@
- decorators^
- XOR~
- bitwise NOT~
as I did not really see too many people doing "bitwise NOT" in their code. Every other character can be seen here and there and can be confusing if this ever causes a problem.Pushed.
Thanks for looking into this. Just got back from Spring Break and it was nice to see something magically removed from my TODO list while I was gone. 🎉
I've been working to remove this preprocess hack for a while because it's been the cause of many bugs, and I don't entirely understand why it's necessary.
In the meantime, this fix seems to work.
Can you take a look at the lint failures? Then I will merge.
[lint:eslint] /home/runner/work/prettier-plugin-ember-template-tag/prettier-plugin-ember-template-tag/tests/unit-tests/preprocess.test.ts
[lint:eslint] 1:1 error Run autofix to sort these imports! simple-import-sort/imports
[lint:eslint] 54:63 error Async arrow function has no 'await' expression @typescript-eslint/require-await
[lint:eslint] 56:17 error Use `for...of` instead of `.forEach(...)` unicorn/no-array-for-each
[lint:eslint] 56:36 error The variable `i` should be named `index`. A more descriptive name will do too unicorn/prevent-abbreviations
Thanks also for setting up the unit tests!
Oh, sorry for not checking the linting. I guess I rely too much on "push & let CI tell me" 🙃
Pushed. The linter gods should be satisfied now @gitKrystan ...
My hunch to as of why this is necessary is that we're working with two (or maybe three) "systems":
code
that is being passed around as simple string.Template
that has items like r.range.start
and range
that need to "align well" with it's own contents
and also the code
from (1).AND
The amount of characters (a, b, c) and multi byte characters has to match in all cases. For example the code breaks in one way if I try to simply count how many bytes are occupied and replace the contents of the template with the same amount of single byte characters. And it then fails in other way when I count the number of characters and replace the template with the same amount of single byte characters.
I suspect (speculation) that for "proper" fix we'd need to:
start
, end
, range
and probably some other properties on the templatestart
, end
and range
in subsequent templates (they had to move, right?)But IDK, that's just a feeling from my experiments.
</div>
or{{/if}}
will be an issue.codeToGlimmerAst
functionality to DRY the code.content
into the replaced template like here, but I could not find a combination that would work. So I believe it's a combination oftemplate.contents
,template.range.end
,template.range.start
operating in different "units" and then something breaks when there is different number of multibyte characters. So I decided to keep thetemplate.contents
as is and use thePLACEHOLDER
instead.PLACEHOLDER = '-';
. Is that a bad idea? All tests are green ...preprocess.ts
to make sure that function in isolation works as expected.