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

Fix compatibility with ember-cli 5.4+ #223

Merged
merged 11 commits into from
Nov 22, 2023
Merged
57 changes: 48 additions & 9 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

relevant background info for the surrounding pre-existing comment

#
# It would be great if vitest had a flag to give us the JSON of all the tests,
# so we could be sure we don't miss anything
# and then generate this list from a previous C.I. job
slow-test:
- defaults with npm
- defaults with yarn
- defaults with pnpm

# TypeScript
- typescript with npm
- typescript with yarn
- typescript with pnpm

# flags
- addon-location
- test-app-location
Expand All @@ -75,3 +68,49 @@ jobs:
- run: pnpm add --global ember-cli yarn
- run: pnpm vitest --testNamePattern "${{ matrix.slow-test }}"
working-directory: tests


defaults_tests:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

name: "ember-cli@${{ matrix.ember-cli }} : ${{ matrix.slow-test }}"
timeout-minutes: 5
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
ember-cli:
- 5.4.1
- 5.3.0

slow-test:
- defaults with npm
- defaults with yarn
- defaults with pnpm
steps:
- uses: actions/checkout@v3
- uses: wyvox/action-setup-pnpm@v2
- run: pnpm add --global ember-cli@${{ matrix.ember-cli }} yarn
- run: pnpm vitest --testNamePattern "${{ matrix.slow-test }}"
working-directory: tests

typescript_tests:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(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 )))

name: "ember-cli@${{ matrix.ember-cli }} : ${{ matrix.slow-test }}"
timeout-minutes: 5
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
ember-cli:
- 5.4.1
# Not testing against 5.3.0, because it has non-passing typechecking
# - 5.3.0

slow-test:
- typescript with npm
- typescript with yarn
- typescript with pnpm
steps:
- uses: actions/checkout@v3
- uses: wyvox/action-setup-pnpm@v2
- run: pnpm add --global ember-cli@${{ matrix.ember-cli }} yarn
- run: pnpm vitest --testNamePattern "${{ matrix.slow-test }}"
working-directory: tests
54 changes: 36 additions & 18 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

);

this.isExistingMonorepo = true;
Expand Down Expand Up @@ -80,10 +80,10 @@ module.exports = {
}

await fs.writeFile(packageJson, JSON.stringify(json, null, 2));
})()
})(),
);

if (options.pnpm) {
if (isPnpm(options)) {
tasks.push(pnpm.createWorkspacesFile(options.target, addonInfo, testAppInfo));
}

Expand All @@ -98,7 +98,7 @@ module.exports = {
await this.moveFilesForExistingMonorepo(options);

this.ui.writeWarnLine(
`Make sure your workspaces are configured correctly to cover the newly created ${addonInfo.location} and ${testAppInfo.location} packages!`
`Make sure your workspaces are configured correctly to cover the newly created ${addonInfo.location} and ${testAppInfo.location} packages!`,
);
}

Expand All @@ -119,7 +119,7 @@ module.exports = {
assert(
!options.addonOnly,
`When in --addon-only mode, we don't need to move files within an existing monorepo. ` +
`If you see this error, please open an issue at: https://github.com/embroider-build/addon-blueprint/issues`
`If you see this error, please open an issue at: https://github.com/embroider-build/addon-blueprint/issues`,
);

let addonInfo = addonInfoFromOptions(options);
Expand Down Expand Up @@ -152,7 +152,7 @@ module.exports = {
assert(
!options.addonOnly,
`When in --addon-only mode, we don't create a test-app. ` +
`If you see this error, please open an issue at: https://github.com/embroider-build/addon-blueprint/issues`
`If you see this error, please open an issue at: https://github.com/embroider-build/addon-blueprint/issues`,
);

const appBlueprint = this.lookupBlueprint('app');
Expand All @@ -179,7 +179,7 @@ module.exports = {
await appBlueprint.install(appOptions);

await Promise.all([
this.updateTestAppPackageJson(path.join(testAppPath, 'package.json'), options.pnpm),
this.updateTestAppPackageJson(path.join(testAppPath, 'package.json'), isPnpm(options)),
this.overrideTestAppFiles(testAppPath, path.join(options.target, 'test-app-overrides')),
fs.unlink(path.join(testAppPath, '.travis.yml')),
]);
Expand All @@ -191,11 +191,11 @@ module.exports = {

if (lt(hasVersion, needsVersion)) {
this.ui.writeWarnLine(
`Your version ${hasVersion} of Ember CLI does not support the --typescript flag yet. Please run \`ember install ember-cli-typescript\` in the ${testAppInfo.location} folder manually!`
`Your version ${hasVersion} of Ember CLI does not support the --typescript flag yet. Please run \`ember install ember-cli-typescript\` in the ${testAppInfo.location} folder manually!`,
);
} else if (lt(hasVersion, recommendedVersion)) {
this.ui.writeWarnLine(
`We recommend using Ember CLI >= ${recommendedVersion} for the best blueprint support when using TypeScript!`
`We recommend using Ember CLI >= ${recommendedVersion} for the best blueprint support when using TypeScript!`,
);
}
}
Expand Down Expand Up @@ -272,21 +272,22 @@ module.exports = {
addonNamespace: addonInfo.name.classified,
blueprintVersion: require('./package.json').version,
year: date.getFullYear(),
yarn: options.yarn,
pnpm: options.pnpm,
npm: options.npm,
packageManager: options.packageManager,
yarn: isYarn(options),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

pnpm: isPnpm(options),
npm: isNpm(options),
typescript: options.typescript,
ext: options.typescript ? 'ts' : 'js',
blueprint: 'addon',
blueprintOptions: buildBlueprintOptions({
[`--addon-location=${options.addonLocation}`]: options.addonLocation,
[`--ci-provider=${options.ciProvider}`]: options.ciProvider,
'--pnpm': options.pnpm,
'--pnpm': isPnpm(options),
'--release-it': options.releaseIt,
[`--test-app-location=${options.testAppLocation}`]: options.testAppLocation,
[`--test-app-name=${options.testAppName}`]: options.testAppName,
'--typescript': options.typescript,
'--yarn': options.yarn,
'--yarn': isYarn(options),
}),
ciProvider: options.ciProvider,
pathFromAddonToRoot,
Expand All @@ -310,7 +311,7 @@ module.exports = {
let ignoredFiles = ['__addonLocation__/tsconfig.json'];

files = files.filter(
(filename) => !filename.match(/.*\.ts$/) && !ignoredFiles.includes(filename)
(filename) => !filename.match(/.*\.ts$/) && !ignoredFiles.includes(filename),
);
}

Expand All @@ -323,10 +324,10 @@ module.exports = {
files = files.filter((filename) => !filename.endsWith('.npmrc'));
}

if (!this.yarn) {
if (!isYarn(options)) {
let ignoredFiles = ['.yarnrc.yml'];

files = files.filter(filename => !ignoredFiles.includes(filename));
files = files.filter((filename) => !ignoredFiles.includes(filename));
}

return files;
Expand All @@ -337,7 +338,7 @@ module.exports = {

if (this.project.isEmberCLIProject() && !this.project.isEmberCLIAddon()) {
throw new SilentError(
'Generating an addon in an existing ember-cli project is not supported.'
'Generating an addon in an existing ember-cli project is not supported.',
);
}

Expand All @@ -360,3 +361,20 @@ function buildBlueprintOptions(blueprintOptions) {

return '';
}

// These methods exist because in ember-cli 5.4, package manager handling
// had changed to solely use the packageManager key, however
// prior to ember-cli 5.4, pnpm, yarn, and npm, had their own booleans on
// the options object.
function isPnpm(options) {
return options.packageManager === 'pnpm' || options.pnpm;
}

function isYarn(options) {
return options.packageManager === 'yarn' || options.yarn;
}

function isNpm(options) {
return options.packageManager === 'npm' || options.npm;
}

39 changes: 9 additions & 30 deletions tests/helpers/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import os from 'node:os';
import path from 'node:path';
import { fileURLToPath } from 'node:url';

import { execa,type Options } from 'execa';
import { execa, type Options } from 'execa';

const DEBUG = process.env.DEBUG === 'true';
const __dirname = path.dirname(fileURLToPath(import.meta.url));
Expand Down Expand Up @@ -36,7 +36,7 @@ const ROLLUP_HASH = /[a-f0-9]{8}/
* a fairly narrow matcher to remove them.
*/
export function withoutHashes(names: string[]) {
return names.filter(name => !ROLLUP_HASH.test(name));
return names.filter(name => !ROLLUP_HASH.test(name));
}

/**
Expand All @@ -52,7 +52,7 @@ export function withoutHashes(names: string[]) {
* This a
*/
export function hashesOnly(names: string[]) {
return names.filter(name => ROLLUP_HASH.test(name));
return names.filter(name => ROLLUP_HASH.test(name));
}

export function splitHashedFiles(names: string[]) {
Expand All @@ -77,33 +77,13 @@ export async function install({
if (packageManager === 'yarn') {
await execa('yarn', ['install', '--non-interactive'], { cwd });
} else {
try {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cleanup tangent.

let installOptions = [];

if (packageManager === 'pnpm') {
installOptions.push('--no-frozen-lockfile');
}

await execa(packageManager, ['install', '--ignore-scripts', ...installOptions], { cwd });
} catch (e) {
if (e instanceof Error) {
// ignore the `@babel/core` peer issue.
// this is dependent on ember-cli-babel doing a v8 release
// where it declares `@babel/core` as a peer, and removes it from
// dependencies
let isPeerError = e.message.includes('Peer dependencies that should be installed:');
let isExpectedPeer = e.message.includes('@babel/core@">=7.0.0 <8.0.0"');

if (packageManager === 'pnpm' && isPeerError && isExpectedPeer) {
console.info('An error occurred. Are there still upstream issues to resolve?');
console.error(e);

return;
}
}

throw e;
let installOptions = [];

if (packageManager === 'pnpm') {
installOptions.push('--no-frozen-lockfile');
}

await execa(packageManager, ['install', '--ignore-scripts', ...installOptions], { cwd });
}

const pkg = await packageJsonAt(cwd);
Expand Down Expand Up @@ -178,7 +158,6 @@ export async function createAddon({

let result = await execa('ember', emberCliArgs, {
...options,
env: { ...options.env, EMBER_CLI_PNPM: 'true' },
preferLocal: true,
});

Expand Down
2 changes: 1 addition & 1 deletion tests/smoke-tests/--addon-location.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe('--addon-location', () => {
tmpDir = await createTmp();

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

Choose a reason for hiding this comment

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

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

args: [`--addon-location=${addonLocation}`, '--pnpm'],
options: { cwd: tmpDir },
});

Expand Down
2 changes: 1 addition & 1 deletion tests/smoke-tests/--test-app-location.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe('--test-app-location', () => {
tmpDir = await createTmp();

let { name } = await createAddon({
args: [`--test-app-location=${testAppLocation}`, '--pnpm=true'],
args: [`--test-app-location=${testAppLocation}`, '--pnpm'],
options: { cwd: tmpDir },
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ for (let packageManager of SUPPORTED_PACKAGE_MANAGERS) {
}

await createAddon({
args: [`--${packageManager}=true`],
args: [`--${packageManager}`],
options: { cwd: tmpDir },
});

Expand Down