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

Switch to pnpm #348

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Jul 21, 2022

This is part 1 of 5 in a series to convert @ember/string to a v2 addon in a less all at once way.
Hopefully this process will demonstrate how others can take individual steps to migrating their addons to the v2 format (aka just a normal npm package)

  1. switch to pnpm <-- you are here
  2. convert to monorepo
  3. extract dummy / tests app to its own package / app
  4. convert addon to v2 addon
  5. convert core test suite to vitest

@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review July 21, 2022 22:21
@NullVoxPopuli NullVoxPopuli force-pushed the switch-to-pnpm branch 2 times, most recently from 5a577cb to e5f4847 Compare July 21, 2022 23:59
"overrides": {
"@octokit/core": "^4.0.0"
},
"overrides-notes": {

Choose a reason for hiding this comment

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

This is super nice. Is override-notes a baked-in pnpm thing or does it just ignore unknown keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It ignores unknown keys - would be cool if this were standardized or something 🤔

package.json Show resolved Hide resolved
Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

This all seems reasonable and good, but I think it's worth getting the @babel/core bits sorted out (@ef4 and @dfreeman). I also marked it as breaking since it drops Node 12.

@@ -9,7 +9,7 @@ Compatibility

* Ember.js v3.20 or above
* Ember CLI v3.20 or above
* Node.js v12 or above
* Node.js v14 or above
Copy link
Contributor

Choose a reason for hiding this comment

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

Just want to flag up: this is breaking! That's fine as long as it's intentional. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is!

node 12 is EOL'd, and pnpm doesn't support it

@NullVoxPopuli
Copy link
Contributor Author

got the @babel/core PR here: emberjs/ember-cli-babel#452

once that lands, the @babel/core in apps will matter, which is ideal 🥳

This is due to a bunch of dependencies not providing it already.
The original Error:

 ERR_PNPM_PEER_DEP_ISSUES  Unmet peer dependencies

.
├─┬ @babel/cli
│ └── ✕ missing peer @babel/core@^7.0.0-0
├─┬ @babel/preset-typescript
│ ├── ✕ missing peer @babel/core@^7.0.0-0
│ └─┬ @babel/plugin-transform-typescript
│   ├── ✕ missing peer @babel/core@^7.0.0-0
│   ├─┬ @babel/helper-create-class-features-plugin
│   │ └── ✕ missing peer @babel/core@^7.0.0
│   └─┬ @babel/plugin-syntax-typescript
│     └── ✕ missing peer @babel/core@^7.0.0-0
├─┬ @ember/test-helpers
│ └─┬ ember-destroyable-polyfill
│   └─┬ ember-compatibility-helpers
│     └─┬ babel-plugin-debug-macros
│       └── ✕ missing peer @babel/core@^7.0.0-beta.42
├─┬ @glimmer/component
│ └─┬ ember-cli-typescript
│   └─┬ @babel/plugin-transform-typescript
│     └── ✕ missing peer @babel/core@^7.0.0-0
├─┬ ember-load-initializers
│ └─┬ ember-cli-typescript
│   ├─┬ @babel/plugin-proposal-class-properties
│   │ └── ✕ missing peer @babel/core@^7.0.0-0
│   └─┬ @babel/plugin-transform-typescript
│     └── ✕ missing peer @babel/core@^7.0.0-0
├─┬ ember-resolver
│ └─┬ babel-plugin-debug-macros
│   └── ✕ missing peer @babel/core@^7.0.0
├─┬ ember-source
│ ├─┬ @babel/plugin-transform-block-scoping
│ │ └── ✕ missing peer @babel/core@^7.0.0-0
│ └─┬ @babel/plugin-transform-object-assign
│   └── ✕ missing peer @babel/core@^7.0.0-0
└─┬ release-it
  └─┬ @octokit/rest
    └─┬ @octokit/plugin-paginate-rest
      └── ✕ unmet peer @octokit/core@>=4: found 3.6.0 in @octokit/rest
Peer dependencies that should be installed:
  @babel/core@">=7.0.0 <8.0.0"
release-it and release-it-lerna-changelog have incompatible peers.

The original error:
```

 ERR_PNPM_PEER_DEP_ISSUES  Unmet peer dependencies

.
└─┬ release-it
  └─┬ @octokit/rest
    └─┬ @octokit/plugin-paginate-rest
      └── ✕ unmet peer @octokit/core@>=4: found 3.6.0 in @octokit/rest

hint: If you don't want pnpm to fail on peer dependency issues, add "strict-peer-dependencies=false" to an .npmrc file at the root of your project.
```
@NullVoxPopuli
Copy link
Contributor Author

Tried rebasing this PR.

pnpm@8 dropped support for Node 14, so we need Node 16, se ... I think it only makes sense to merge this PR if we're going to commit to the v2 conversion (which would be a breaking change anyway, due to 3.0.1 not having ember-auto-import as a requirement).

@ef4
Copy link
Contributor

ef4 commented Apr 7, 2023

I would like to suggest that the future of @ember/string is to tell people to just stop using it. There's little benefit is destabilizing it with breaking changes. It only exists for compat.

@NullVoxPopuli
Copy link
Contributor Author

Why is it considered de-stabilization to make it a v2 addon?

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Apr 7, 2023

The inflection library seems pretty good:

-import { camelize, capitalize } from '@ember/string';
+import { camelize, capitalize } from 'inflection';

@ef4
Copy link
Contributor

ef4 commented Apr 7, 2023

Why is it considered de-stabilization to make it a v2 addon?

I just mean what you said here:

which would be a breaking change anyway

@NullVoxPopuli
Copy link
Contributor Author

ah yeah,

which would be a breaking change anyway

I'm making an assumption here -- so this could be wrong -- but I think maybe some of the uncertainty around v2 conversions is around keeping main/master releasable due to lack of time dedicated to maintain the projects -- imo, when it comes to v2 addon conversions, I don't think it's worth keeping main/master releasable.
it's a good ideal, but there are too many simultaneous changes (in this repo and many others) needed to complete the v2 conversion that would mean multiple majors would need to be published before v2 conversion is complete -- and that is very disruptive!!

Keeping main/master releasable could resume post-v2 conversion, which generally would include the following breaking changes:

  • node support (node ceases to matter, actually, so in terms of "bundling changes", rather than keeping main/master releasable, dropping node support isn't a consumer-facing change at all)
  • auto-import@v2 is required
  • explicit peers are declared (ember-source, glimmer component/tracking, etc)

Keeping main/master releasable is greatly beneficial when projects don't have a lot of maintenance efforts behind them or if folks are busy, etc.

But, I have the time and energy to push all the v2 addon conversions forward (one repo at a time), so if I were unblocked, the time of having unreleasable main/master would not be that long, and risk'd be low, and there would be only a single breaking change release with the above possible breaking changes.

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Apr 13, 2023

bah, inflection doesn't support dashes.

nerdsniped myself dreamerslab/node.inflection#95

It's possible the maintainer does not want to support kebab case: dreamerslab/node.inflection#72

Made these tests: https://runkit.com/nullvoxpopuli/is-inflection-sufficient
Found that change-case performs a bit better: https://runkit.com/nullvoxpopuli/64381760e1828400085cfffc, but not exactly what I want. easy enough to fix that trailing _ though.

@mansona
Copy link
Member

mansona commented Dec 13, 2023

So I generally agree with @NullVoxPopuli on this one, I think there is overall benefit to upgrading this to v2 not just for Ember people but for people who are integrating Ember code that uses this package into other ecosystems. Before a v2 conversion this is just not possible

As for breaking changes. I think we should convert this repo to use release-plan and block the merging of the release PR with a "request changes" until all the work is done. The only real breaking change is the requirement to use ember-auto-import@2 for this since as @NullVoxPopuli points out because after the v2 conversion this will no longer be dependent on node version 👍

If we're ok with this as a plan I'll open a PR to move to release-plan. How does that sound?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants