Fix compatibility with ember-cli 5.4+ #223

NullVoxPopuli · 2023-10-31T21:52:39Z

Disruption caused by: ember-cli/ember-cli#10368
We'll need to test with ember-cli 5.3 and 5.4

Fixes:

--pnpm, --npm, --yarn flags not propagating correctly
- X=true is no longer allowed in ember-cli
- using packageManager === X to detect which package manager is in use
  - helpers extracted to support pre ember-cli 5.4
ember-cli 5.4.1 fixes the typescript tests, so CI now specifically calls that out

CI has been updated to test against 5.3 and 5.4.1

ci now also only tests TS against 5.4.1
ember-cli 5.3 TS is broken, so we won't test that

Of note,

it looks like CI isn't enforcing our lint/formatting rules. This isn't good because editors typically do enforce these.

Clearer matrix name

Updates test

…al minor release (instead of major) for @tsconfig/ember

…ber-cli 5.4

…esting with

…only test against 5.4.1

…he default tests

NullVoxPopuli · 2023-11-21T18:14:46Z

.github/workflows/ci.yml

@@ -44,19 +44,12 @@ jobs:
        # waiting on a an api from vitest for querying
        # the list of tests ahead of time before running them.
        #
+        # https://github.com/vitest-dev/vitest/issues/2901


relevant background info for the surrounding pre-existing comment

NullVoxPopuli · 2023-11-21T18:15:15Z

.github/workflows/ci.yml

@@ -75,3 +68,49 @@ jobs:
      - run: pnpm add --global ember-cli yarn
      - run: pnpm vitest --testNamePattern "${{ matrix.slow-test }}"
        working-directory: tests
+
+
+  defaults_tests:


these tests are probably the most important to have anything "matrixy" -- no frills, just defaults

NullVoxPopuli · 2023-11-21T18:15:44Z

.github/workflows/ci.yml

+      - run: pnpm vitest --testNamePattern "${{ matrix.slow-test }}"
+        working-directory: tests
+
+  typescript_tests:


typescript tests get a separate matrix, due to reasons
(accidental non-breaking release of @tsconfig/ember in this case)

(this is only needed because our tests ensure that lint passes across both workspaces)
(( but we don't actually have any control over the test app ))

((( which should be fine under normal circumstances )))

NullVoxPopuli · 2023-11-21T18:17:13Z

index.js

@@ -39,7 +39,7 @@ module.exports = {
    if (!options.addonOnly) {
      if (fs.existsSync(path.join('..', 'package.json'))) {
        options.ui.writeInfoLine(
-          "Existing monorepo detected! The blueprint will only create the addon and test-app folders, and omit any other files in the repo's root folder."
+          "Existing monorepo detected! The blueprint will only create the addon and test-app folders, and omit any other files in the repo's root folder.",


my editor added these commas -- I think this repo's prettier config is probably not being checked in ci or something. didn't look too deeply in to it.

NullVoxPopuli · 2023-11-21T18:18:11Z

index.js

-      pnpm: options.pnpm,
-      npm: options.npm,
+      packageManager: options.packageManager,
+      yarn: isYarn(options),


because we now have to check for X as well as packageManager === X, these are now extracted utils at the bottom of the file.

For my understanding: this change in ember-cli is only not a breaking change for users, because things like --pnpm are still supported as an alias, but it is a breaking change for us, because --pnpm is not exposed to us as options.pnpm anymore but as options.packageManager (as of 5.4). Is that correct?

NullVoxPopuli · 2023-11-21T18:18:41Z

tests/helpers/utils.ts

@@ -77,33 +77,13 @@ export async function install({
  if (packageManager === 'yarn') {
    await execa('yarn', ['install', '--non-interactive'], { cwd });
  } else {
-    try {


cleanup tangent.

NullVoxPopuli · 2023-11-21T18:19:03Z

tests/smoke-tests/--addon-location.test.ts

@@ -20,7 +20,7 @@ describe('--addon-location', () => {
    tmpDir = await createTmp();

    let { name } = await createAddon({
-      args: [`--addon-location=${addonLocation}`, '--pnpm=true'],


ember-cli 5.4 doesn't support X=true, and it turns out it wasn't needed, even for 5.3

lolmaus

Thanks!! 🙀

simonihmig · 2023-11-22T11:01:29Z

index.js

-      pnpm: options.pnpm,
-      npm: options.npm,
+      packageManager: options.packageManager,
+      yarn: isYarn(options),


For my understanding: this change in ember-cli is only not a breaking change for users, because things like --pnpm are still supported as an alias, but it is a breaking change for us, because --pnpm is not exposed to us as options.pnpm anymore but as options.packageManager (as of 5.4). Is that correct?

NullVoxPopuli · 2023-11-22T11:47:16Z

Is that correct?

yup!

Cleanup from a long time ago

c6f8983

NullVoxPopuli force-pushed the update-ci branch from 3f1f393 to c6f8983 Compare October 31, 2023 22:00

NullVoxPopuli mentioned this pull request Nov 10, 2023

add quotes around path in GitHub workflow #227

Merged

NullVoxPopuli mentioned this pull request Nov 21, 2023

Add missing files to --addon-only output #230

Merged

NullVoxPopuli added 2 commits November 21, 2023 12:09

Update ci.yml

024b762

Clearer matrix name

Use packageManager

11011ef

Updates test

NullVoxPopuli force-pushed the update-ci branch from 5204fe6 to 11011ef Compare November 21, 2023 17:10

NullVoxPopuli added 3 commits November 21, 2023 12:20

Add another fixture for TS and notes about why -- related to accident…

573767d

…al minor release (instead of major) for @tsconfig/ember

Refactor isPnpm and isX utilities

d45147a

Don't pass pnpm=true when pnpm works, and setting a value confuses em…

bf8c247

…ber-cli 5.4

NullVoxPopuli added internal bug Something isn't working and removed internal labels Nov 21, 2023

NullVoxPopuli changed the title ~~Let's see what's wrong with CI / ember-cli 5.4~~ Fix compatibility with ember-cli 5.4+ Nov 21, 2023

NullVoxPopuli added 4 commits November 21, 2023 12:51

No need to set EMBER_CLI_PNPM since min ember-CLI is 5.3 that we're t…

3f13572

…esting with

With ember-cli 5.4.1 release, the TS tests should be fixed now if we …

44456d1

…only test against 5.4.1

change matrix around to only test different ember-cli versions with t…

b994364

…he default tests

=true is not allowed for --pnpm, --yarn, --npm, etc

13c6108

NullVoxPopuli commented Nov 21, 2023

View reviewed changes

Missed an isPnpm usage

e247293

NullVoxPopuli commented Nov 21, 2023

View reviewed changes

NullVoxPopuli marked this pull request as ready for review November 21, 2023 18:19

NullVoxPopuli requested review from lolmaus, simonihmig and void-mAlex November 21, 2023 22:57

lolmaus approved these changes Nov 22, 2023

View reviewed changes

simonihmig approved these changes Nov 22, 2023

View reviewed changes

NullVoxPopuli merged commit cfcf7cc into main Nov 22, 2023
18 checks passed

NullVoxPopuli deleted the update-ci branch November 22, 2023 11:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compatibility with ember-cli 5.4+ #223

Fix compatibility with ember-cli 5.4+ #223

NullVoxPopuli commented Oct 31, 2023 •

edited

Loading

NullVoxPopuli Nov 21, 2023

NullVoxPopuli Nov 21, 2023

NullVoxPopuli Nov 21, 2023

NullVoxPopuli Nov 21, 2023

NullVoxPopuli Nov 21, 2023

NullVoxPopuli Nov 21, 2023

lolmaus Nov 22, 2023

simonihmig Nov 22, 2023

NullVoxPopuli Nov 21, 2023

NullVoxPopuli Nov 21, 2023

lolmaus left a comment

simonihmig Nov 22, 2023

NullVoxPopuli commented Nov 22, 2023

Fix compatibility with ember-cli 5.4+ #223

Fix compatibility with ember-cli 5.4+ #223

Conversation

NullVoxPopuli commented Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lolmaus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Nov 22, 2023

NullVoxPopuli commented Oct 31, 2023 •

edited

Loading