Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ember v3.28.0...v5.2.0 #402

Merged
merged 1 commit into from
Sep 11, 2023
Merged

Ember v3.28.0...v5.2.0 #402

merged 1 commit into from
Sep 11, 2023

Conversation

mixonic
Copy link
Contributor

@mixonic mixonic commented Sep 8, 2023 •
edited
Loading

  • Upgrade Ember CLI from 3.28 -> 5.2
  • Upgrade Ember source from 3.12 -> 5.2
  • Upgrade Ember data from 3.12 -> 4.12

Also

  • Adds Volta
  • Bumps Node to v16
  • Drop Ember versions < 3.28 from the CI
  • Add Ember 5 to the CI
  • Address many code style issues after the Ember CLI upgrade
  • Drop ember-code-snippet, which is old, unmaintained, and not future safe, in favor of a very lame hand-written snippet system.
  • Some fixes to the docs pages, which seemed to have gone broken. They are not pretty, but everything works.
  • Drop ember-cli-fastboot. It was demanding additional dependencies in order to work correctly which I do not think should be added as library level dependencies. Seems like something to address as/if we split out the docs site (or I will later make an ember-try scenario for the docs site generation if ember-cli-fastboot is a requirement for that, I don't think it is)

@mixonic mixonic mentioned this pull request Sep 8, 2023
Closed
@mixonic mixonic force-pushed the mixonic/bump-ember-cli branch 8 times, most recently from 639dcbe to 8bb133d Compare September 9, 2023 00:27
@mixonic mixonic added the 5.0 label Sep 9, 2023
@mixonic mixonic marked this pull request as ready for review September 9, 2023 00:29
@mixonic
Copy link
Contributor Author

mixonic commented Sep 9, 2023

FYI to reviewers, this would start the breaking changes on the road to a v5: #400

@mixonic mixonic changed the title v3.28.0...v5.2.0 Ember v3.28.0...v5.2.0 Sep 9, 2023
runspired
runspired reviewed Sep 10, 2023
View reviewed changes
CONTRIBUTING.md Outdated Show resolved Hide resolved
runspired
runspired reviewed Sep 10, 2023
View reviewed changes
index.js
@@ -62,8 +62,6 @@ module.exports = {
disableDecoratorTransforms: false,
enableTypeScriptTransform: true,

throwUnlessParallelizable: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason we didn't leave this? if we aren't parallelizable something bad has gone wrong. I've worked pretty hard to keep things parallelizable around the ecosystem opening fixes when hitting issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to trace the error back to the root cause. I can give it another look, but I'm going to consider this non-blocking and decouple the investigation. Frankly I considered this option as being old enough that I wasn't sure it was still something meaningful.

A conversion of this whole project to a v2 addon would simplify the dependencies and I presume make the build parallelization concerns moot (at least for consumers of this library), and I suspect that is the meta.

runspired
runspired reviewed Sep 10, 2023
View reviewed changes
tests/dummy/app/routes/application/template.hbs
@@ -1,8 +1,8 @@
<header class="container-fluid">
<div class="row">
<h1 class="col-sm-6">
<LinkTo @route="index">
<img alt="HTML Next Logo" src="./HTML-Next.png" style="height: 2em;">
<LinkTo @route="index" style="color:#222">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we end up needing the extra color?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link is an a tag and turns blue, but this is the logo on the top left in the header. We want to to be the same color as the logo image, which is #222.

This was probably broken in 3343ecc#diff-a6b32449eac6431ef7263de086a744f8aed3883a686fe93880e607f5e08ffdb9L3, although I don't believe the docs site after that change was ever deployed (it doesn't seem like it, at least).

I agree this is a bit lame, but I think you agree we have bigger fish to fry.

runspired
runspired reviewed Sep 10, 2023
View reviewed changes
tests/dummy/app/routes/examples/infinite-scroll/controller.js
},

loadBelow() {
let last = this.model.last;
let last = this.model.data.last;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry for the sins that I committed

runspired
runspired reviewed Sep 10, 2023
View reviewed changes
tests/integration/modern-ember-test.js
@@ -23,8 +22,6 @@ module('vertical-collection', 'Integration | Modern Ember Features Tests', funct
</div>
`);

await settled();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I dislike this lint rule as its actually dishonest. But also yay cleanup

runspired
runspired approved these changes Sep 10, 2023
View reviewed changes
Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no blocking feedback, left a few nits inline.

@mixonic mixonic force-pushed the mixonic/bump-ember-cli branch 2 times, most recently from d037092 to c659a6c Compare September 11, 2023 14:41
@mixonic mixonic merged commit a52ab83 into master Sep 11, 2023
11 checks passed
@mixonic mixonic deleted the mixonic/bump-ember-cli branch September 11, 2023 15:07
@mixonic mixonic mentioned this pull request Sep 11, 2023
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@runspired runspired runspired approved these changes

@RobbieTheWagner RobbieTheWagner Awaiting requested review from RobbieTheWagner

Assignees
No one assigned
Labels
5.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mixonic @runspired