Updated dependencies and ember-try scenarios #408

ijlee2 · 2024-11-27T15:21:42Z

Background

While investigating blockers for introducing Embroider, I realized that files in the addon folder (w/ version 4.1.4) show older Ember syntax and folder layout. This means, projects that depend on ember-modal-dialog will be unable to update their ember-source to 6.0.0.

I'd like to introduce 1-2 patch pull requests, to help these projects more independently introduce Embroider, introduce Ember v6, and migrate away from ember-modal-dialog. To do so, we need CI for the master branch to pass again.

What changed?

Updated dependencies to the latest, whenever possible.
Updated v1 addon setup to match closely the more recent setup from Ember CLI.
Replaced ember-prism with ember-shiki and downgraded liquid-* packages, so that tests will run without build or runtime errors (without these changes, we can see these errors when we run the dummy app locally and attempt to open a modal).
Updated ember-try scenarios to test ember-lts-3.28, ember-lts-4.12, ember-lts-5.12, Ember 6.x, and embroider-safe.

Note

ember-lts-3.24 has to be skipped because ember-shiki uses named blocks.

ember-release, ember-beta, and ember-canary (all point to 6.x) are failing because the addon has deprecated syntax ({{action}} helper) and classic component layout. Let's mark the failures as passing for now, since they should be fixed in the follow-up patch PRs.

Tip

I recommend checking the commits one-by-one using the Commits tab. You can select Hide whitespace to see fewer changes.

ijlee2 · 2024-11-27T15:26:10Z

patches/[email protected]

+-import { service } from '@ember/service';
+import { inject as service } from '@ember/service';


I patched ember-shiki because the syntax import { service } from '@ember/service' doesn't work in ember-lts-3.28.

ijlee2 · 2024-11-27T15:27:03Z

tests/acceptance/animatable-test.js

@@ -21,7 +21,7 @@ module('Acceptance: modal-dialog | animatable', function (hooks) {
  test('basic modal', async function (assert) {
    assert.dom(modalRootElementSelector).exists({ count: 1 });
    assert.isAbsent(overlaySelector);
-    assert.dom('#example-basic button').exists({ count: 1 });
+    assert.dom('#example-basic button').exists({ count: 2 });


The number of button's changed to 2 because ember-shiki has a copy button. To change the number back to 1, consider using data-test-* selectors instead of IDs and class names.

ijlee2 · 2024-11-27T15:35:49Z

.github/workflows/ci.yml

          - ember-release
          - ember-beta
          - ember-canary
          - embroider-safe
-          - embroider-optimized
+          # - embroider-optimized


I saw that ember-modal-dialog passes embroider-safe. It's unlikely that it'll ever pass embroider-optimized due to liquid-* packages.

lukemelia · 2024-11-27T15:50:52Z

Thanks for tackling this @ijlee2. Once you get to your goal of green on CI, I'm happy to merge.

ijlee2 · 2024-11-27T15:58:45Z

@lukemelia Thanks. Earlier, I ran lint, test, and test:ember-compatibility locally, and they all passed. I see now that CI for this pull request failed due to a permission issue:

Please make sure you have the correct access rights
and the repository exists.

pnpm: Command failed with exit code 128: /usr/bin/git clone [email protected]:rwwagner90/perf-primitives.git /home/runner/.local/share/pnpm/store/v3/tmp/_tmp_1782_a3d77804b6ff0bdc785b16c3[22](https://github.com/yapplabs/ember-modal-dialog/actions/runs/12053373763/job/33610243999?pr=408#step:6:23)b4bd85
Cloning into '/home/runner/.local/share/pnpm/store/v3/tmp/_tmp_1782_a3d77804b6ff0bdc785b16c322b4bd85'...
[email protected]: Permission denied (publickey).
fatal: Could not read from remote repository.

~~Is this something that you could help look into?~~

I see now. The error is due to my using liquid-wormhole from pzuraq's branch. I'll look into possible alternatives tomorrow.

…ndencies) with ember-shiki

… app

ijlee2 · 2024-11-28T07:00:55Z

package.json

+  "pnpm": {
+    "overrides": {
+      "perf-primitives": "0.0.6"
+    },


The error that I posted in #408 (comment) occurred because liquid-wormhole@8cfb67b declared a forked commit of perf-primitives as its dependency.

While it may be possible to configure this repo's GitHub Actions to allow installing dependencies from forks, for simplicity, I think it may be better to force the original perf-primitives to be installed.

It's worth noting that, because tests for the animatable route pass only with unofficial versions of liquid-tether and liquid-wormhole, a passing CI doesn't necessarily mean that this addon will work on a consuming project with liquid-* packages.

ijlee2 · 2024-11-28T07:06:35Z

@lukemelia Could you try running CI again? I'm not sure if it'll pass this time, because a couple of dev-dependencies (liquid-tether and liquid-wormhole) still refer to external repos.

lukemelia · 2024-12-02T01:06:09Z

@ijlee2 Good to merge from your POV?

ijlee2 · 2024-12-02T07:30:05Z

Good to merge from your POV?

I think so. On Friday, I noticed that, in contrast to what can be currently seen in the deployed site, the animations for some modals in the tethered-animatable route don't work. I couldn't find the right combinations of versions to install to get the animations to work and to get tests to run and pass.

Since these modals in tethered-animatable can still be opened/closed and the addon is now in maintenance mode, I think it's okay to merge this pull request so that we can focus on getting the CI green and removing deprecated syntax.

ijlee2 added 2 commits November 27, 2024 16:10

chore: Updated dependencies

5701069

chore: Updated configurations for dummy app

7d6b770

ijlee2 force-pushed the update-dependencies branch from 21e7c06 to 4c314da Compare November 27, 2024 15:24

ijlee2 commented Nov 27, 2024

View reviewed changes

ijlee2 marked this pull request as draft November 27, 2024 16:05

ijlee2 added 13 commits November 28, 2024 07:25

chore: Simplified scripts for lint and test

cc00cba

chore: Updated lint configurations

567b85b

chore: Ran lint:hbs:fix

3043a26

chore: Ran lint:js:fix

6b62e45

chore: Ignored remaining lint errors

55e6dcb

refactor: Removed classic layout from dummy app

ee455c3

chore: Replaced ember-prism (deprecated, incompatible with newer depe…

462529c

…ndencies) with ember-shiki

chore: Patched ember-shiki to run on older Ember versions

6675879

bugfix: Downgraded liquid-* packages to versions set in tag 4.1.4

b89f661

chore: Updated ember-try scenarios

474b107

chore: Added lockfile

085ee42

bugfix: Forced the original perf-primitives to be installed for dummy…

bbf3f34

… app

chore: Updated lockfile

9e57388

ijlee2 force-pushed the update-dependencies branch from 4c314da to 9e57388 Compare November 28, 2024 06:53

ijlee2 commented Nov 28, 2024

View reviewed changes

ijlee2 marked this pull request as ready for review November 28, 2024 07:01

ijlee2 merged commit 6a62491 into yapplabs:master Dec 2, 2024
9 checks passed

ijlee2 deleted the update-dependencies branch December 2, 2024 07:30

lukemelia added chore internal labels Dec 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated dependencies and ember-try scenarios #408

Updated dependencies and ember-try scenarios #408

ijlee2 commented Nov 27, 2024 •

edited

Loading

ijlee2 Nov 27, 2024 •

edited

Loading

ijlee2 Nov 27, 2024 •

edited

Loading

ijlee2 Nov 27, 2024 •

edited

Loading

lukemelia commented Nov 27, 2024

ijlee2 commented Nov 27, 2024 •

edited

Loading

ijlee2 Nov 28, 2024

ijlee2 commented Nov 28, 2024

lukemelia commented Dec 2, 2024

ijlee2 commented Dec 2, 2024

		-import { service } from '@ember/service';
		+import { inject as service } from '@ember/service';

Updated dependencies and ember-try scenarios #408

Updated dependencies and ember-try scenarios #408

Conversation

ijlee2 commented Nov 27, 2024 • edited Loading

Background

What changed?

ijlee2 Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

ijlee2 Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

ijlee2 Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

lukemelia commented Nov 27, 2024

ijlee2 commented Nov 27, 2024 • edited Loading

ijlee2 Nov 28, 2024

Choose a reason for hiding this comment

ijlee2 commented Nov 28, 2024

lukemelia commented Dec 2, 2024

ijlee2 commented Dec 2, 2024

ijlee2 commented Nov 27, 2024 •

edited

Loading

ijlee2 Nov 27, 2024 •

edited

Loading

ijlee2 Nov 27, 2024 •

edited

Loading

ijlee2 Nov 27, 2024 •

edited

Loading

ijlee2 commented Nov 27, 2024 •

edited

Loading