Support 'ember-cli-htmlbars' import by default#293
Support 'ember-cli-htmlbars' import by default#293izelnakri wants to merge 4 commits intoember-cli:masterfrom
Conversation
izelnakri
force-pushed
the
support-ember-cli-htmlbars
branch
from
d3fede6 to
f84220d
Compare
rwjblue
left a comment
rwjblue
left a comment
There was a problem hiding this comment.
I think its totally good to sync the default value for module with the ones provided by ember-cli-htmlbars:
I'm not sure we need to duplicate the tests, but also seems totally fine.
index.js
Outdated
| throw path.buildCodeFrameError(msg); | ||
| } | ||
| } else { | ||
| } else if (modulePathExport !== 'hbs') { |
There was a problem hiding this comment.
Why is this change required? ember-cli-htmlbars already configures the modules option for us (and it works just fine without this else if change).
There was a problem hiding this comment.
Currently this PR does that as well on line 96. This line change allows for errors to be thrown if user/developer writes:
import hbs from 'ember-cli-htmlbars';while it should be:
import { hbs } from 'ember-cli-htmlbars';We might be able to find a different way to accomplish this. I was kind of satisfied with a single line change considering it passed both versions of tests and performance implications would be minimal(single if check). I'm of course willing to change it if we can find a better way :)
There was a problem hiding this comment.
The issue is that we shouldn't hard code the "hbs" name here (the names that are allowed should be based on the modules config)
There was a problem hiding this comment.
@rwjblue updated the the code, tests passing now. Also did few manual tests just to be sure 😅
What does this mean? This PR adds support for transpiling |
Yes, sorry for the wording. I meant the new change wont degrade the performance much since it adds a single if check. |
|
|
||
| let modules = state.opts.modules || { | ||
| 'htmlbars-inline-precompile': 'default', | ||
| 'ember-cli-htmlbars': 'hbs', |
There was a problem hiding this comment.
#292 claims that ember-cli-htmlbars currently is not supported but due to the state.opts.modules || part of this statement I'm wondering if that's really the case. what's stopping you from passing in a modules option from the outside?
There was a problem hiding this comment.
I wanted to also throw this error and make sure tests passes with correct imports on both cases: #293 (comment) . Also this results in better defaults, allows any developer to use it without passing that opts, which makes sense because that really should be the default behaviour practically speaking.
There was a problem hiding this comment.
what's stopping you from passing in a modules option from the outside?
FWIW, I mentioned this somewhere else, but that is exactly what we are doing in ember-cli-htmlbars
Either way, it seems totally fine to have this package default the configuration to match what we expect folks to actually use in real life.
added transpiler support for import { hbs } from 'ember-cli-htmlbars';
izelnakri
force-pushed
the
support-ember-cli-htmlbars
branch
from
e434d83 to
a170185
Compare
|
@rwjblue thanks for your reviews! Is there anything else blocking this PR? I would like to get it merged otherwise. |
3 participants
This PR adds a performant transpiler support for
import { hbs } from 'ember-cli-htmlbars.Extensive tests are also provided/adjusted for this behavior.
The reasoning behind this PR is described here: #292