Upgrade for ember 3.x; remove babel warning; remove bower;#111
Conversation
|
ps - w/o updating ember-cli-changelog upstream, optionalDependencies was used to remove the babel 5x warning also, there is still 1 ember global used when gaining access to the EventDispatcher, which ember-views exports, but only internally within ember itself. Open to suggestions here cc @rwwagner90 |
| import layout from '../templates/components/fast-action'; | ||
| import { htmlSafe } from "@ember/string"; | ||
|
|
||
| const { SafeString } = Ember.Handlebars; |
| import layout from '../templates/components/fast-async'; | ||
| import asyncAction from './async-element'; | ||
|
|
||
| const { SafeString } = Ember.Handlebars; |
|
|
||
| const { | ||
| EventDispatcher, | ||
| } = Ember; |
There was a problem hiding this comment.
Ember global remains, how do we get EventDispatcher using new module syntax - its exported internally from ember-views within ember itself, but I don't find an external export?
| "ember-cli-version-checker": "^2.1.0", | ||
| "ember-getowner-polyfill": "^2.2.0", | ||
| "hammerjs": "^2.0.8", | ||
| "marked": "~0.3.5", |
There was a problem hiding this comment.
where is this used? it's a lot of code? (brought over from bower)
There was a problem hiding this comment.
moved app.import to ember-cli-build, shouldn't bloat size of code shipped to consumers
where its used, but its not used by those who install this addon, that we can know for sure)
eriktrom
force-pushed
the
chore/upgrade-3-1
branch
from
7d11bf1 to
39a8207
Compare
|
I'm going to leave this for a day - it's a huge commit - someone may realize it may break something in the next 24 hours. If not, i'll merge and release then. |
| "configPath": "tests/dummy/config" | ||
| } | ||
| }, | ||
| "optionalDependencies": {} |
There was a problem hiding this comment.
Please remove this empty optionalDependencies
|
|
||
| if (environment === 'production') { | ||
| if (environment === 'production') {} // eslint-disable-line | ||
|
|
There was a problem hiding this comment.
We should put a comment in this block saying it is intentionally an empty block, rather than disabling lint.
| files: ['AnimationFrame.js'], | ||
| }); | ||
|
|
||
| let markedTree = new Funnel(path.dirname(require.resolve('marked')), { |
There was a problem hiding this comment.
Where are we getting these vendor files from? I thought you said we were not pulling in marked to apps either? What do we use it for?
|
@rwwagner90 - thanks for your thorough feedback. I updated per your comments. Regarding marked. I removed it in the last commit. I looked through the codebase and there is no sign of its use. Also all 3 of us didn't know why it was installed in the first place. I will publish this evening, unless there is any further feedback, in case this does cause a breaking change and a consumer is not using a lock file while trying to say, finish a friday sprint. |
3 participants
fixes #108
fixes #110