chore: add missing `@ember/string` dependency to peers

[Techn1x]

Resolves https://github.com/emberjs/ember-inflector/issues/347

This addon uses @ember/string but it is not listed in its package json file.

With strictier package managers these days (like pnpm) we need to make sure this is correct.

This addon should have little opinion on the ember/string version used, so I've allowed for v3 or v4 in the peer dependencies.

[mkszepp]

this should fix build error when using @ember/string v4

Build Error (PackagerRunner) in ../rewritten-packages/ember-inflector.1e27a29e/node_modules/ember-inflector/lib/system/inflector.js

Module not found: Error: ember-inflector is trying to import the app's @ember/string package, but it seems to be missing

Stack Trace and Error Report: /tmp/error.dump.bf7b185fd84496f74cc6bd4383d96fef.log

[NullVoxPopuli]

@Techn1x can you push again? I just re-enabled CI -- so we'll be able to see if this is a safe change to make

[Techn1x]

sure! pushed

[NullVoxPopuli]

Hm, in order to make sure we don't break things, we need ci to be green first.

Would you be willing to fix it in separate prs?

[Techn1x]

Oh no!

I can take a look tomorrow (Australia 🦘). Ideally I wanted this to be a patch so that downstream consumers just get it without much hassle

But to get CI working it looks like we would need to drop some old versions of node, I hope it's not much more complicated than that. But I think that would then require cutting a major, and downstream consumers aren't likely to get that updated quickly (would need to update dependencies of repos like mirage)

Any advice? Is that the best way to go?

Maybe we first make this a smaller PR that just lists the ember/string V3 dependency, cut a patch release (assuming it is OK), then a major which widens to ember/string 4 and drops old node versions?

[NullVoxPopuli]

But to get CI working it looks like we would need to drop some old versions of node

I wonder if it's possible to pin whatever requires newer node? since, I assume at one point, CI was green?

[Techn1x]

I'm not sure main branch was ever green at any point from what I can see, but nonetheless I'll see what I can figure out! Hopefully I can just pin a dependency. The V2 pr there that is green might give a clue as well

[mkszepp]

I think the problem is here: https://github.com/emberjs/ember-inflector/blob/7da42ab62a9ef05c79d06ee4a284b28c8a4a4240/package.json#L89-L92

maybe replacing to v16 helps already to get green ci?

error @embroider/shared-internals@1.8.3: The engine "node" is incompatible with this module. Expected version "12.* || 14.* || >= 16". Got "15.3.0"

@embroider/shared-internals doesn't allow anymore node v15... maybe it was working time ago

[NullVoxPopuli]

so the v2 conversion PR (I was just re-looking at it) does have green CI -- but would also be a breaking change -- and it also needs to add @ember/string to dependencies/peerdeps https://github.com/emberjs/ember-inflector/pull/499/files#r1672417937

so in order to get a fix out for ember-inflector in a non-breaking release, we need to not make the breaking change here (else just make all the breaking changes at once, such as in @mansona's v2 PR)

:-\

doesn't allow anymore node v15... maybe it was working time ago

I fear that the only way around stuff like this is increasingly aggressive overrides/resolutions on the super old versions on those libraries

[mkszepp]

tested shortly updating node to v16... with that change you get already some tests green, but not all.. the problem is, you run into issue of ember-auto-import for ember-release, ember-beta... because app is using atm auto-import v1... When updating auto-import to v2 we run into next error (error from @embroider/macros -> You gave us a visitor for the node type ClassAccessorProperty but it's not a valid type.

Looks like bringing this to the finish will be more work...

[NullVoxPopuli]

yea, we'd probably have to remeove ember-release/beta/canary from the try matrix, and/or swap them out with what actual versions would have been

Looks like bringing this to the finish will be more work...

<3 compatibility often is monotonous

[mansona]

you can rebase this now that https://github.com/emberjs/ember-inflector/pull/502 is merged 👍

[Techn1x]

Wow, a lot happened while I was sleeping! 😄

Rebased!

[mansona]

so looking at the failures and trying to figure out exactly what the situation is 🫠 I think our best bet is to actually just make this have a dependency on @ember/string@3 rather than playing with the peers

if we want this to be a non-breaking fix then I think that's the only way forward 😞

[Techn1x]

I'm not so sure, in order to be using this addon, users would have had to have ember/string installed in their projects right? So a peer makes sense?

I think we just have to add ember/string to those ember-try scenarios?

[mansona]

I think you might be right, it's worth testing... But it would then need to be a breaking change anyway 🤔

[Techn1x]

Sorry for the commit mess, have been doing this from my phone.

Added 5.4 and 5.8 try scenarios to make sure they work, and fixed up ember 3.x scenarios by listing ember/string 3.

It seems that for ember 5.x onwards they need to use ember/string 3, based on an experiment branch I have elsewhere. So I figured I would remove ember/string 4 for this work and leave that to the major, but this is where I had to stop for now, can't run npm install from a phone 😛 I'll be at my desk in a few hours.

Regardless, I'd like to propose a radical alternative;

• can we instead drop ember/string from this addon? It looks remarkably easy.

This addon already has its own regex for camelize etc. so let's just replace this capitalize usage with the same regex from ember/string?

https://github.com/emberjs/ember-inflector/blob/420e91087361c6b32dcc9a0c8d876b01ba852e29/addon/lib/system/inflector.js#L313

Regex https://github.com/emberjs/ember-string/blob/5393b5ed308a49fc00596c7ed5d0ec5be4d758f9/src/index.ts#L83

[Techn1x]

I'll put together a seperate PR to see what that looks like as an alternative. It should be the safest, least hassle option

[Techn1x]

Done! I reckon this is a better way forward https://github.com/emberjs/ember-inflector/pull/505

[mansona]

fixed in #505