[Cleanup] Remove all traces of hbs #146
Conversation
NullVoxPopuli
changed the title
There was a problem hiding this comment.
We discussed this in the embroider meeting and we generally agree that this is a good thing to merge.
While it would make the linked "splitting" issue easier to implement we don't need to have this PR merged before we're going to start that work 👍 It would just be nice to have the newly split lib not need to support something that is "the old way to do something" 😂
|
|
||
| return preprocessEmbeddedTemplates(string, config).output; | ||
| if (relativePath.match(/\.(gjs|gts)$/)) { |
There was a problem hiding this comment.
When will this condition ever not match?
There was a problem hiding this comment.
I specifically did 0 refactoring so the pr would be more digestible.
Changing existing logic would be out of scope for this, and require more testing as well.
There was a problem hiding this comment.
Can clean up in quick follow up PR
There was a problem hiding this comment.
This conditional is directly tied to the line you changed above, turning {js,gjs,ts,gts} into just {gjs,gts}, so this isn't so much "refactoring" as "keeping the code consistent with the other changes you made". If you're planning on doing another pass in a separate PR, though, then I'm not going to lose sleep over it.
There was a problem hiding this comment.
tied to the line you changed above
that is a good point!
then I'm not going to lose sleep over it.
thanks, yeah, it's just folks across OSS review so infrequently, I don't want to to overwhelm anyone with a bunch of changes, even if they are all needed.
Update snapshots since data has been removed Commit lockfile changes due to package.json change Fix case where <template> could be within a template string (found in REPL) Add failing test for situation brought up on limber.glimdown.com Add fix for reported issue on limber.glimdown.com Fix comment in babel-plugin.js
NullVoxPopuli
force-pushed
the
remove-hbs-literals
branch
from
9ba5399
to
f19dbba
Compare
Background and motivation:
ember-template-imports was originally intended to be an experiment for all (and more!) possibilities outlined in emberjs/rfcs#779 (and explored through this blog series: https://v5.chriskrycho.com/journal/ember-template-imports/ ).
Of those possibilities, ember-template-imports implemented 2:
hbs``- https://v5.chriskrycho.com/journal/ember-template-imports/part-3/<template>- what was eventually chosen via RFC 779So, since RFC 779 chose
<template>, we no longer needhbs``, which this PR removes.To help simplify the work of #143, we want to remove as much of the old stuff as possible before we extract the preprocessor and the babel plugin to their own locations.
This is most def a breaking change.
Though, there is no rush on cutting a release, and we could even wait until the extraction outlined in #143 is complete.
This PR is just removal, to minimize the diff.
There are some obvious refactors / de-type unioning that can happen afterwards.