Add support for npm and pnpm #36
Conversation
NullVoxPopuli
force-pushed
the
workspaces-support
branch
from
c7db3e4 to
b4f65b2
Compare
NullVoxPopuli
force-pushed
the
workspaces-support
branch
from
2092db9 to
8112dd3
Compare
NullVoxPopuli
force-pushed
the
workspaces-support
branch
from
756a96a to
3ce4e71
Compare
NullVoxPopuli
changed the title
NullVoxPopuli
force-pushed
the
workspaces-support
branch
from
abbb7bd to
00da80a
Compare
NullVoxPopuli
changed the title
NullVoxPopuli
force-pushed
the
workspaces-support
branch
from
daf093e to
0498e5a
Compare
NullVoxPopuli
force-pushed
the
workspaces-support
branch
from
f9aa7aa to
84cb53b
Compare
NullVoxPopuli
changed the title
| @@ -42,6 +42,7 @@ | |||
| "eslint-plugin-ember": "^10.5.8", | |||
| "eslint-plugin-node": "^11.1.0", | |||
| "eslint-plugin-prettier": "^4.0.0", | |||
| "prettier": "^2.5.1", | |||
There was a problem hiding this comment.
eslint-plugin/config-prettier declare a prettier peer
NullVoxPopuli
changed the title
NullVoxPopuli
changed the title
tests/utils.ts
Outdated
| if (packageManager === 'pnpm') { | ||
| console.info('An error occurred. Are there still upstream issues to resolve?'); | ||
| console.error(e); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
It seems to me this special handling of pnpm here was probably mostly useful during development. Do we still need this, or can we just remove the catch-block?
Also, even if we want some special error message for pnpm, shouldn't this still throw? This is now returning "successfully" even when execa throws.
There was a problem hiding this comment.
We do need this, because the app-blueprint is not peer-ily correct. The start of resolving that is here:
There was a problem hiding this comment.
Both pnpm and npm support disabling the strict peer dep checking via .npmrc. If that is the reason for your special error handling, we should just use that setting instead.
There was a problem hiding this comment.
given our discussion about doing some more involved maintenance on ember-cli-babel, and doing a major over there, I went ahead and did the very specific error catching in this PR so we don't over-catch errors
| let packageJson = path.join(options.target, 'package.json'); | ||
| let json = await fs.readJSON(packageJson); | ||
|
|
||
| json.scripts = scripts(options); |
There was a problem hiding this comment.
We are overriding the "template-based" scripts section now AFAICT? So the actual lint scripts like lint:js and lint:hbs are missing now, aren't they?
I think we have to decide if we want to split defining the scripts setup using both this programmatic approach and the blueprint "template", or just the former. In the first case, we would need to merge them here (and probably sort by key), in the latter we need to define all scripts in the root-package-json.js helper and remove them from the template (they are still in there, albeit unused).
Given tests are passing here, it also shows our testing approach is not complete yet. Beside these "smoke tests" we currently have (which run e.g. the lint script, which is now passing without actually linting anything), we probably need also some more traditional unit/integration like tests similar to what other blueprints do, testing the output against expected fixtures. Not in this PR of course, just mentioning here...
There was a problem hiding this comment.
So the actual lint scripts like lint:js and lint:hbs are missing now, aren't they?
these already don't exist in the root package.json -- I'm matching pairity with the existing files/package.json: https://github.com/embroider-build/addon-blueprint/blob/main/files/package.json#L16
, it also shows our testing approach is not complete yet.
yup! lots more to do
testing the output against expected fixtures. Not in this PR of course, just mentioning here...
yeah, I can look in to doing this in a followup -- these are "snapshot tests with extra steps", which help catch accidental / unexpected changes -- which I think we're now ready for testing against
There was a problem hiding this comment.
ah also, until package.json .. or node in general supports jsonc, the templating we use in blueprints will not-so-great at documenting our context / thoughts / "why" / etc in the blueprint files.
|
I agree with @simonihmig that the pnpm error handling should change before this can merge. Instead of ignoring exceptions there is a precise way to disable the strict peer dep checking. |
|
except we don't want to disable strict peer dep checking, so that's not exactly a solution. We want strict peer dep checking. The only problem is the |
|
I agree. But the current exception-trapping approach already ignores strict peer dep failures. My suggestion is to only ignore strict peer dep failures while allowing any other pnpm error to propagate. When the underlying blueprint is corrected we would remove the setting, same as what you're suggesting with removing the exception trap. |
…an ignore all errors under pnpm
|
I've updated the PR to do specific error-catching, rather than over-error catching. ready for re-review, @simonihmig and @ef4 <3 |
The current released version of the addon-blueprint only provides
yarn.This PR:
scriptscommands/behavior between the 3 package managers the sameCloses: #35