-
Notifications
You must be signed in to change notification settings - Fork 28
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
Changes from all commits
c6f8983
024b762
11011ef
573767d
d45147a
bf8c247
3f13572
44456d1
b994364
13c6108
e247293
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
# | ||
# 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 | ||
|
@@ -75,3 +68,49 @@ jobs: | |
- run: pnpm add --global ember-cli yarn | ||
- run: pnpm vitest --testNamePattern "${{ matrix.slow-test }}" | ||
working-directory: tests | ||
|
||
|
||
defaults_tests: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typescript tests get a separate matrix, due to reasons There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (this is only needed because our tests ensure that ((( 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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)); | ||
} | ||
|
||
|
@@ -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!`, | ||
); | ||
} | ||
|
||
|
@@ -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); | ||
|
@@ -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'); | ||
|
@@ -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')), | ||
]); | ||
|
@@ -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!`, | ||
); | ||
} | ||
} | ||
|
@@ -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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: 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, | ||
|
@@ -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), | ||
); | ||
} | ||
|
||
|
@@ -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; | ||
|
@@ -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.', | ||
); | ||
} | ||
|
||
|
@@ -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; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)); | ||
|
@@ -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)); | ||
} | ||
|
||
/** | ||
|
@@ -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[]) { | ||
|
@@ -77,33 +77,13 @@ export async function install({ | |
if (packageManager === 'yarn') { | ||
await execa('yarn', ['install', '--non-interactive'], { cwd }); | ||
} else { | ||
try { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
@@ -178,7 +158,6 @@ export async function createAddon({ | |
|
||
let result = await execa('ember', emberCliArgs, { | ||
...options, | ||
env: { ...options.env, EMBER_CLI_PNPM: 'true' }, | ||
preferLocal: true, | ||
}); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,7 @@ describe('--addon-location', () => { | |
tmpDir = await createTmp(); | ||
|
||
let { name } = await createAddon({ | ||
args: [`--addon-location=${addonLocation}`, '--pnpm=true'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 }, | ||
}); | ||
|
||
|
There was a problem hiding this comment.
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