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

breaking: drops Ember < 3.28, upgrades to Ember 4.12, supports Ember 5 #741

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

breaking: drops Ember < 3.28, upgrades to Ember 4.12, supports Ember 5 #741

wants to merge 14 commits into from

Conversation

Pixelik
Copy link

@Pixelik Pixelik commented Nov 1, 2023 •
edited
Loading

Breaking

drops Ember < 3.28

drops Node < 16.14

  • because of lru-cache installed by ember-source

Chore

upgrades to Ember 4.12

supports Ember 5

  • by removing import { assign } from '@ember/polyfills' which no longer exists in Ember 5

@Pixelik Pixelik changed the title chore: removes deprecated assign polyfill chore(ember-5-compatible): removes deprecated assign polyfill Nov 1, 2023
@Pixelik Pixelik changed the title chore(ember-5-compatible): removes deprecated assign polyfill chore: makes addon Ember 5 compatible Nov 1, 2023
@Pixelik
Copy link
Author

Pixelik commented Nov 1, 2023

ping @nlfurniss 🙏 🙂

void-mAlex
void-mAlex reviewed Nov 3, 2023
View reviewed changes
addon/utils/mung-options-for-fetch.ts Outdated
@@ -13,7 +12,7 @@ import {
export default function mungOptionsForFetch(
options: AjaxOptions
): FetchOptions {
const hash = assign(
const hash = Object.assign({},
Copy link

Choose a reason for hiding this comment

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

I think we may need to drop support for ember < 4 to do this as 3.28 still supports IE 11

Copy link
Author

@Pixelik Pixelik Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

PR updated

Copy link
Member

Choose a reason for hiding this comment

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

Is that true 🤔 this just moves out of the concern of the addon and could be handled by a babel plugin or something

Copy link
Member

Choose a reason for hiding this comment

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

essentially it would be bad if we dropped support for 3.28 for no reason

Copy link
Author

Choose a reason for hiding this comment

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

ember-fetch does not work on Ember 5 without this change

Copy link
Member

Choose a reason for hiding this comment

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

sorry I wasn't 100% clear 🙈 we should make this change, but we shouldn't stop testing in Ember 3.28

Copy link
Author

@Pixelik Pixelik Nov 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I've updated the PR.
I had to remove support for Ember < 3.28 because of ember-qunit but that's about it.
So this PR now supports Ember >= 3.28 (including Ember 5).
I've refactored the code so that neither the ember assign polyfill nor Object.assign() are used in production code. So IE is also still supported.

@Pixelik Pixelik changed the title chore: makes addon Ember 5 compatible breaking: upgrades to Ember 4 / drops Ember 3 / makes addon Ember-5-compatible Nov 6, 2023
@mansona mansona mentioned this pull request Nov 9, 2023
Open
@Pixelik Pixelik force-pushed the chore/ember-5-compatible branch 2 times, most recently from 98e00f1 to e95211a Compare November 9, 2023 22:30
@Pixelik Pixelik changed the title breaking: upgrades to Ember 4 / drops Ember 3 / makes addon Ember-5-compatible breaking: upgrades to Ember 4, drops Ember 3 & makes it Ember-5-compatible Nov 10, 2023
@Pixelik Pixelik force-pushed the chore/ember-5-compatible branch 2 times, most recently from 8c13f95 to 7e6860b Compare November 10, 2023 12:46
@Pixelik Pixelik changed the title breaking: upgrades to Ember 4, drops Ember 3 & makes it Ember-5-compatible breaking: upgrades to Ember 4, drops Ember 3, supports Ember 5 Nov 10, 2023
@Pixelik Pixelik force-pushed the chore/ember-5-compatible branch 7 times, most recently from 68b337e to a34cd17 Compare November 10, 2023 13:25
@Pixelik Pixelik changed the title breaking: upgrades to Ember 4, drops Ember 3, supports Ember 5 breaking: upgrades to Ember 4, drops Ember 3, drops IE, supports Ember 5 Nov 10, 2023
@Pixelik Pixelik force-pushed the chore/ember-5-compatible branch 2 times, most recently from e9e915a to 9bec330 Compare November 10, 2023 14:15
@Pixelik Pixelik changed the title breaking: upgrades to Ember 4, drops Ember 3, drops IE, supports Ember 5 breaking: drops Ember < 3.28, upgrades to Ember 4, supports Ember 5 Nov 10, 2023
@Pixelik Pixelik changed the title breaking: drops Ember < 3.28, upgrades to Ember 4, supports Ember 5 breaking: drops Ember < 3.28, upgrades to Ember 4.12, supports Ember 5 Nov 10, 2023
@Pixelik Pixelik force-pushed the chore/ember-5-compatible branch 2 times, most recently from c0239d8 to 0c7dc29 Compare November 11, 2023 13:13
@Pixelik Pixelik force-pushed the chore/ember-5-compatible branch 7 times, most recently from dee80b2 to 5316d6d Compare November 13, 2023 01:05
@void-mAlex void-mAlex mentioned this pull request Nov 13, 2023
Open
@Pixelik
Copy link
Author

Pixelik commented Nov 14, 2023

All tests are passing except for 5 mocha tests failing during yarn test:node 🤷‍♂️

@Pixelik
Copy link
Author

Pixelik commented Nov 14, 2023 •
edited
Loading

ping @xg-wang 🙏 ☝️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mansona mansona mansona left review comments

@void-mAlex void-mAlex Awaiting requested review from void-mAlex

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

Successfully merging this pull request may close these issues.
3 participants
@Pixelik @mansona @void-mAlex