-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[doc beta] fixing references to Ember in the subdirectory ember #16164
[doc beta] fixing references to Ember in the subdirectory ember #16164
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is complaining about ember/tests/error_handler_test.js: "TestAdapter" is read-only
import { run } from 'ember-metal'; | ||
import { jQuery } from 'ember-views'; | ||
import { QueryParamTestCase, moduleFor } from 'internal-test-helpers'; | ||
import { inject } from '@ember/service'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change this to import { inject as service } from '@ember/service';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@locks Done 😄
@@ -1,9 +1,11 @@ | |||
import Ember from 'ember'; | |||
import { run } from 'ember-metal'; | |||
import { DEBUG } from 'ember-env-flags'; | |||
import TestAdapter from '@ember/test/adapter'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem quite right, as CI is failing throughout all jobs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@locks This one I wasn't completely sure because of this Ember.Test.adapter = ADAPTER
. Maybe that's the reason why CI complains. Should I revert it? or is it safe to remove the const ADAPTER
and the check if (Ember.Test)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really updating live source code is not really the scope of #15723 and I'd be fine just updating inline docs here (and possibly assert/error messages).
@@ -20,7 +22,7 @@ QUnit.module('error_handler', { | |||
Ember.testing = TESTING; | |||
window.onerror = WINDOW_ONERROR; | |||
if (Ember.Test) { | |||
Ember.Test.adapter = ADAPTER; | |||
TestAdapter = ADAPTER; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe imported variables are treated as const and you can't assign to them like this.
@@ -200,8 +200,10 @@ Object.defineProperty(Ember, 'LOG_BINDINGS', { | |||
and reporting code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
line 390 refers to Ember.String.loc
. Those types of refernces are what we are looking to update to String.loc
As per your question, only mappings detailed here https://github.com/ember-cli/ember-rfc176-data/blob/master/mappings.json |
Thanks for your review @toddjordan! I've made the corrections according to your comments. I couldn't replace
Here is the link to the failed build in Travis. |
@MarcoUmpierrez Great! I think this is good as is. Like I mentioned I'm fine not changing any prod imports with this pr and sticking with the inline docs (and some messages). The last thing I'd ask if is if you could use git to squash the commits into one and prepend the commit comment with Let me know if you need help with this. |
@toddjordan done! |
Thanks @MarcoUmpierrez !! |
In relation to this issue #15723, I have fixed the references to Ember in the subdirectory
ember
.There were some things I wasn't sure about; I list them here so anyone with more experience can guide me to replace them correctly:
lib/index.js: here appears
Ember.String.loc()
in a comment, but I wasn't sure if I should replace it with just "loc()
helper" or "loc()
helper in@ember/string
".tests/error_handler_test.js:
Ember.onerror
has no replacement as far as I know. I researched it and found this comment of rwjblue. The same happens withEmber.testing
.Ember.Test
. Should Iimport Test from 'test'
?I think the rest should be ok, but I would appreciate if someone can check it to make sure that I didn't anything wrong 😄