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

Techn1x · 2024-07-08T06:17:26Z

Resolves #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 · 2024-07-08T08:44:59Z

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 · 2024-07-09T18:18:23Z

@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 · 2024-07-10T05:03:23Z

sure! pushed

NullVoxPopuli · 2024-07-10T13:01:40Z

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 · 2024-07-10T13:15:57Z

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 · 2024-07-10T13:32:53Z

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 · 2024-07-10T13:39:59Z

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 · 2024-07-10T14:06:39Z

I think the problem is here:

ember-inflector/package.json

Lines 89 to 92 in 7da42ab

    
           "volta": { 
        
             "node": "15.3.0", 
        
             "yarn": "1.22.10" 
        
           }

maybe replacing to v16 helps already to get green ci?

error @embroider/[email protected]: 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 · 2024-07-10T14:42:17Z

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 · 2024-07-10T15:18:43Z

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 · 2024-07-10T15:21:03Z

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 · 2024-07-10T16:49:15Z

you can rebase this now that #502 is merged 👍

Techn1x · 2024-07-11T02:35:26Z

Wow, a lot happened while I was sleeping! 😄

Rebased!

mansona · 2024-07-11T11:13:27Z

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 · 2024-07-11T11:48:54Z

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 · 2024-07-11T12:18:59Z

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

NullVoxPopuli · 2024-07-11T16:56:02Z

package.json

@@ -37,7 +37,7 @@
    "@babel/eslint-parser": "^7.21.3",
    "@babel/plugin-proposal-decorators": "^7.21.0",
    "@ember/optional-features": "^2.0.0",
-    "@ember/string": "^3.0.1",
+    "@ember/string": "^4.0.0",


@ember/string @ v4 can't support ember-source < 3.28

So... it might be impossible to use @ember/string @ v4 until we do a modern version of ember-inflector (which @mansona already has a PR for)

NullVoxPopuli · 2024-07-11T16:56:46Z

package.json

@@ -37,7 +37,7 @@
    "@babel/eslint-parser": "^7.21.3",
    "@babel/plugin-proposal-decorators": "^7.21.0",
    "@ember/optional-features": "^2.0.0",
-    "@ember/string": "^3.0.1",
+    "@ember/string": "^4.0.0",


I wonder though if it would work if we set this to @ember/string: ^3.0.0 || ^4.0.0 and configured the exact version in the try config.

also, I feel like someane already said that, and I'm too scattered today :(

Update ember-try.js

tests/dummy/config/ember-try.js

Update ember-try.js

Update ci.yml

Techn1x · 2024-07-11T20:24:32Z

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?

ember-inflector/addon/lib/system/inflector.js

Line 313 in 420e910

substitution = capitalize(substitution);

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

Techn1x · 2024-07-11T20:35:55Z

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 · 2024-07-12T06:43:53Z

Done! I reckon this is a better way forward #505

mansona · 2024-07-12T09:52:59Z

fixed in #505

Techn1x force-pushed the ember-string-peerdep branch from 646fcad to a0c581c Compare July 10, 2024 05:02

phndiaye approved these changes Jul 10, 2024

View reviewed changes

chore: add missing ember/string dependency

9796fb7

Techn1x force-pushed the ember-string-peerdep branch from a0c581c to 9796fb7 Compare July 11, 2024 02:34

NullVoxPopuli reviewed Jul 11, 2024

View reviewed changes

Techn1x added 2 commits July 12, 2024 05:33

Update ember-try.js

07e6bcd

Merge pull request #2 from Techn1x/patch-1

e3086e3

Update ember-try.js

Techn1x commented Jul 11, 2024

View reviewed changes

tests/dummy/config/ember-try.js Outdated Show resolved Hide resolved

Techn1x added 5 commits July 12, 2024 05:35

Update tests/dummy/config/ember-try.js

d28e896

Update ember-try.js

0b982a2

Merge pull request #3 from Techn1x/patch-2

1cabca0

Update ember-try.js

Update ci.yml

77266de

Merge pull request #4 from Techn1x/patch-3

0aa93d9

Update ci.yml

mansona closed this Jul 12, 2024

bjornharrtell mentioned this pull request Sep 16, 2024

Missing dependency on @ember/string yapplabs/ember-modal-dialog#402

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Techn1x commented Jul 8, 2024 •

edited

Loading

mkszepp commented Jul 8, 2024

NullVoxPopuli commented Jul 9, 2024

Techn1x commented Jul 10, 2024

NullVoxPopuli commented Jul 10, 2024 •

edited

Loading

Techn1x commented Jul 10, 2024 •

edited

Loading

NullVoxPopuli commented Jul 10, 2024

Techn1x commented Jul 10, 2024 •

edited

Loading

mkszepp commented Jul 10, 2024 •

edited

Loading

NullVoxPopuli commented Jul 10, 2024

mkszepp commented Jul 10, 2024 •

edited

Loading

NullVoxPopuli commented Jul 10, 2024

mansona commented Jul 10, 2024

Techn1x commented Jul 11, 2024

mansona commented Jul 11, 2024

Techn1x commented Jul 11, 2024

mansona commented Jul 11, 2024

NullVoxPopuli Jul 11, 2024

NullVoxPopuli Jul 11, 2024

Techn1x commented Jul 11, 2024 •

edited

Loading

Techn1x commented Jul 11, 2024

Techn1x commented Jul 12, 2024

mansona commented Jul 12, 2024

chore: add missing @ember/string dependency to peers #501

chore: add missing @ember/string dependency to peers #501

Conversation

Techn1x commented Jul 8, 2024 • edited Loading

mkszepp commented Jul 8, 2024

NullVoxPopuli commented Jul 9, 2024

Techn1x commented Jul 10, 2024

NullVoxPopuli commented Jul 10, 2024 • edited Loading

Techn1x commented Jul 10, 2024 • edited Loading

NullVoxPopuli commented Jul 10, 2024

Techn1x commented Jul 10, 2024 • edited Loading

mkszepp commented Jul 10, 2024 • edited Loading

NullVoxPopuli commented Jul 10, 2024

mkszepp commented Jul 10, 2024 • edited Loading

NullVoxPopuli commented Jul 10, 2024

mansona commented Jul 10, 2024

Techn1x commented Jul 11, 2024

mansona commented Jul 11, 2024

Techn1x commented Jul 11, 2024

mansona commented Jul 11, 2024

NullVoxPopuli Jul 11, 2024

Choose a reason for hiding this comment

NullVoxPopuli Jul 11, 2024

Choose a reason for hiding this comment

Techn1x commented Jul 11, 2024 • edited Loading

Techn1x commented Jul 11, 2024

Techn1x commented Jul 12, 2024

mansona commented Jul 12, 2024

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

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

Techn1x commented Jul 8, 2024 •

edited

Loading

NullVoxPopuli commented Jul 10, 2024 •

edited

Loading

Techn1x commented Jul 10, 2024 •

edited

Loading

Techn1x commented Jul 10, 2024 •

edited

Loading

mkszepp commented Jul 10, 2024 •

edited

Loading

mkszepp commented Jul 10, 2024 •

edited

Loading

Techn1x commented Jul 11, 2024 •

edited

Loading