ember-remodal icon indicating copy to clipboard operation
ember-remodal copied to clipboard

Upgrade ember cli to 3.x

Open sethbrasile opened this issue 6 years ago • 3 comments

Closes #45

This is not ready but I wanted to get the PR going so I could see if Travis is happy with it and etc...

@esbanarango if you get a chance, feel free to look around and let me know if you see any issues, or feel free to fix any you see. Also, if you wouldn't mind, would you consider being the "boss" of this one? I've been away for too long to know if this is good or trash, and I've been away for too long to know the right way to release this and handle versioning.

I made some decisions that may or may not have been good ones feel free to revert them, or ask me to do it:

  • This was my first experience with yarn, and I didn't like how yarn and npm felt like they were fighting. Since ember-cli seems to now favor yarn, I removed package.json.lock
  • I didn't fix the ember-component.send-action deprecation, because I'm not sure if there's a way to avoid breaking changes. I think if we switched to closure actions, it would end up breaking ember-remodal's API? I guess if this was going to be a major, breaking changes could be ok, but I didn't want to make that decision by myself. Since it's only a deprecation for now, it doesn't necessarily have to be included in this PR? Thoughts?
  • I allowed ember-cli to make as many changes as I could allow, to keep code up-to-date in general
  • With addition of .template-lintrc.js came a ton of template lint warnings. I fixed a ton of them (single quotes and etc..), but I didn't want to "fix" everything so I disabled a few of the rules.
  • I added jQuery explicitly since we're using it in the ember-remodal component.
  • I removed 4 tests because they were broken (I think it was runloop/timing stuff) and they weren't totally aligning with coverage needs anyway
  • I didn't take any existing issues into consideration, so some of those may need to be addressed before an actual release?
  • I added @babel/core and [email protected] to devDependencies in order to silence peer dependency warnings
  • I think I intended on removing the er components before another major release, but it's been too long for me to remember or know if that's a good idea

One more note: I'm not currently developing any apps, so I don't have much current real-world perspective. Hopefully I'm not too far removed for this to be valuable!

sethbrasile avatar Dec 28 '18 16:12 sethbrasile

@esbanarango I don't know what this means:

4:23  error  "resolve" is not published               node/no-unpublished-require
5:22  error  "broccoli-funnel" is not published       node/no-unpublished-require
6:26  error  "broccoli-merge-trees" is not published  node/no-unpublished-require

resolve's current version on npm is 1.9.0 so I'm not sure how it's not published?

sethbrasile avatar Dec 28 '18 16:12 sethbrasile

Got it! It's telling me that those packages need to be in dependencies and not devDependencies

whoops!

sethbrasile avatar Dec 28 '18 16:12 sethbrasile

@esbanarango since I'm not certain if those are actually needed in the build, I wonder if they should go back in devDependencies and then we silence the error instead? Not sure.

sethbrasile avatar Dec 28 '18 16:12 sethbrasile