Cleanup EmberApp class #6795
Cleanup EmberApp class #6795
Conversation
aka. better IDE navigation 😉
| options.sourceMapConfig = this.options.sourcemaps; | ||
|
|
||
| return concat(tree, options); | ||
| this.project.ui.writeDeprecateLine('EmberApp.concatFiles() is deprecated. Please use the `broccoli-concat` module directly.'); |
There was a problem hiding this comment.
This unconditionally deprecates calling this.concatFiles (and I presume refactors the internals to avoid calling it and instead call _concatFiles). We originally still funneled work through this.concatFiles because of a ember-cli-fastboot monkey patch. I'm happy to remove this, but we need to also update that monkey patch to continue to function after this lands.
FWIW, the fastboot team has a path forward to remove the need for this monkey patch, and @kratiahuja has been working hard on landing. Take a peek at the meeting summary where we came to consensus...
There was a problem hiding this comment.
and I presume refactors the internals to avoid calling it and instead call _concatFiles
we're actually already calling _concatFiles() everywhere internally. the only call to concatFiles() is in a unit test that tests that the deprecation message is being printed.
There was a problem hiding this comment.
@rwjblue I guess once this lands we need to update the monkey patch until we ship the new fastboot build scheme right?
There was a problem hiding this comment.
@kratiahuja - Yes, but as @Turbo87 pointed out it is already broken (because this PR didn't remove any calls to this.concatFiles, meaning that we had already broken the fastboot monkey patch).
There was a problem hiding this comment.
I actually don't see how this change would result in different behavior for anyone monkeypatching these methods. It still works the same as before and it still shows the same deprecation message as before.
There was a problem hiding this comment.
Ohio sorry my bad, I didn't see you are still calling concatFiles on the next line. Yeah then it will work.
| @@ -679,23 +674,21 @@ class EmberApp { | |||
| @return {Tree} | |||
| */ | |||
| _filterAppTree() { | |||
| if (this._cachedFilterAppTree) { | |||
| return this._cachedFilterAppTree; | |||
| if (!this._cachedFilterAppTree) { | |||
There was a problem hiding this comment.
Hehe, I preferred the simpler guard + return clause. No real reason to force rightward shift for such a simple thing.
Not blocking though, this is also fine...
There was a problem hiding this comment.
I'm fine with either, but what I definitely don't like is stuff like return this.cachedSomething = something; aka. assignments in return statements. I actually thought I enabled the ESLint rule that prevented stuff like that
There was a problem hiding this comment.
@Turbo87 - Seems good to disable return with assignment (I agree many folks are confused by these types of statements).
| @@ -742,16 +733,16 @@ class EmberApp { | |||
| */ | |||
| _configReplacePatterns() { | |||
| return [{ | |||
| match: /\{\{rootURL\}\}/g, | |||
There was a problem hiding this comment.
apparently not, no. my IDE actually told me that it's not needed and it seems to work fine without the escaping.
There was a problem hiding this comment.
Sweet! Not sure why I would have put them there originally. 😿
|
@homu r+ |
|
📌 Commit 41501d8 has been approved by |
|
☀️ Test successful - status |
No description provided.