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.

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

Update ember-resources from v5 to v7 #281

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

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Sep 5, 2024

Via

npx ember-resources-codemod

Here is the code for the codemod:
NullVoxPopuli/ember-resources#1147


There aren't many breaking changes from v5 to v7.

Here are the breaking changes in v6:

Here are the breaking changes in v7:


Of note, because ember-resources has such broad support (3.28 - 6+), ember-resources declares a peer on ember-source so that it can correctly choose which features to use based on the consuming app's version.

As a result, ember-resources (in pnpm7 anyway), ember-resources must also become a peer, which is a breaking change. (However, I'm exploring if peers can be removed entirely here https://discord.com/channels/480462759797063690/568935504288940056/1281690947708784681 -- which would make this PR not a breaking change (once I propagate the findings to the relevant libraries))

There is a fair bit of this code:

import { dependencySatisfies, importSync, macroCondition } from '@embroider/macros';

let getOwner: (context: unknown) => Owner | undefined;
let setOwner: (context: unknown, owner: Owner) => void;

if (macroCondition(dependencySatisfies('ember-source', '>=4.12.0'))) {
  getOwner = importSync('@ember/owner').getOwner;
  setOwner = importSync('@ember/owner') .setOwner;
} else {
  getOwner = importSync('@ember/application').getOwner;
  setOwner = importSync('@ember/application').setOwner;
}

The default ember-source for ember-headless-table is v4, but it looks like 3.28+ is also supported (the same range as ember-resources).

Though, while addressing this, I ran in to a number of peer problems. It may be best to address those in separate PRs, but they are included here in hopes that CI goes green, since I hadn't yet contributed to this repo since my time at CS.
image

I also ran in to a number of maintenance problems that will probably not mean this PR will go green

  • out of date dependencies:
    • @embroider/*
    • ember-cli
    • ember-auto-import
    • pnpm

These alone are not problematic except when wanting to have fixed build problems.
In particular, I noticed with the current repo's dependencies, the dependency graph is incorrect, and packages are being resolved incorrectly -- for example, something is trying to resolve reactiveweb/dist/index, but that file doesn't exist, and nor should it -- because you only pay for what you import, and having a barrel file for a generic library would be bad for performance.

So, I guess this PR is more demonstration than anything, as other work must happen before this work can attempt to go green.

(If I used this library, I'd PR fixes for ya'll -- but I haven't had a need for a table in a while -- my timebox ran out on trying to get this repo working <3 )

Copy link

changeset-bot bot commented Sep 5, 2024

🦋 Changeset detected

Latest commit: 80a14ef

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
ember-headless-table Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@NullVoxPopuli NullVoxPopuli changed the title Update ember-resources from v6 to v7 Update ember-resources from v5 to v7 Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant