Skip to content

fix: Remove ember-cli-string-helpers #1672

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

Merged

Conversation

MichalBryxi
Copy link
Contributor

@MichalBryxi MichalBryxi commented Apr 23, 2025

Core

  • This pulls @ember/string 3.1.1 into dependent project and that causes a lot of issues
  • Since we have capitalize in @ember/string, we can just call that in our internal helper
  • Also bumped internal version of @ember/string to v4.x -> I could not make it work, next step

Fixed tests

  • Also pins portal target for ember-tether to correct scope for testing so that assertions can work correctly

Fixes #1666

@MichalBryxi
Copy link
Contributor Author

MichalBryxi commented Apr 23, 2025

This PR can be merged (CI will get green) once #1671 is merged.

@MichalBryxi MichalBryxi force-pushed the mb/1666-remove-ember-string branch 3 times, most recently from d522b5f to ffd89a8 Compare May 2, 2025 13:35
@MichalBryxi
Copy link
Contributor Author

I'm bumping into surprising amount of hurdles. This bundles multiple fixes along the way of the core of removing ember-cli-string-helpers. Unfortunately the wanted bump of @ember/string to >4.x is not happening as I can't figure out how to make pnpm ember try:one embroider-safe tests pass.

It always goes down to something like:

not ok 10 Chrome 135.0 - [2298 ms] - Acceptance | Search: search works for guides pages
    ---
        stack: >
            Error: Could not find module `@ember/string` imported from `(require)`
                at missingModule (http://localhost:7357/assets/vendor.js:254:11)
                at findModule (http://localhost:7357/assets/vendor.js:265:7)
                at requireModule (http://localhost:7357/assets/vendor.js:31:15)
                at eval (webpack://dummy/?../../.pnpm/@[email protected]_@[email protected][email protected][email protected]/node_modules/@embroider/babel-loader-9/index.js?%7B%22variant%22:%7B%22name%22:%22dev%22,%22runtime%22:%22browser%22,%22optimizeForProduction%22:false%7D,%22appBabelConfigPath%22:%22/Users/michal/ember/ember-cli-addon-docs/node_modules/.embroider/rewritten-app/_babel_config_.js%22,%22cacheDirectory%22:%22/private/var/folders/8q/yrjx_8s115q43r8w5qd90_m00000gn/T/embroider/webpack-babel-loader%22%7D!../../.pnpm/@[email protected]_@[email protected][email protected]/node_modules/@embroider/webpack/src/virtual-loader.js?f=%252F%2540embroider%252Fext-cjs%252F%2540ember%252Fstring&a=%252FUsers%252Fmichal%252Fember%252Fember-cli-addon-docs%252Ftests%252Fdummy:1:18)
                at ../../.pnpm/@[email protected]_@[email protected][email protected][email protected]/node_modules/@embroider/babel-loader-9/index.js?{"variant":{"name":"dev","runtime":"browser","optimizeForProduction":false},"appBabelConfigPath":"/Users/michal/ember/ember-cli-addon-docs/node_modules/.embroider/rewritten-app/_babel_config_.js","cacheDirectory":"/private/var/folders/8q/yrjx_8s115q43r8w5qd90_m00000gn/T/embroider/webpack-babel-loader"}!../../.pnpm/@[email protected]_@[email protected][email protected]/node_modules/@embroider/webpack/src/virtual-loader.js?f=%2F%40embroider%2Fext-cjs%2F%40ember%2Fstring&a=%2FUsers%2Fmichal%2Fember%2Fember-cli-addon-docs%2Ftests%2Fdummy! (http://localhost:7357/assets/chunk.d71aec03172c45bf0f60.js:307:1)
                at __webpack_require__ (http://localhost:7357/assets/chunk.bc91aea9c094d989cd00.js:915:42)
                at eval (webpack://dummy/../rewritten-packages/ember-modal-dialog.7c0702fc/node_modules/ember-modal-dialog/components/modal-dialog.js?:8:71)
                at ../rewritten-packages/ember-modal-dialog.7c0702fc/node_modules/ember-modal-dialog/components/modal-dialog.js (http://localhost:7357/assets/chunk.83d522583eb6e55007d5.js:5605:1)
                at __webpack_require__ (http://localhost:7357/assets/chunk.bc91aea9c094d989cd00.js:915:42)

And I can't figure out why it can't find said package in the app bundle when doing the embroider specific build. For non-embroider builds this works ok.

@MichalBryxi
Copy link
Contributor Author

Anywho: Ready for review @SergeAstapov

@@ -1,32 +1,26 @@
{{#if this.media.isMobile}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@MichalBryxi did this leak somehow as pr title only says about ember-cli-string-helpers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry, yes. First this PR needs to be merged: #1676

@MichalBryxi MichalBryxi changed the title fix: Remove ember-cli-string-helpers [WIP] fix: Remove ember-cli-string-helpers May 2, 2025
- This pulls @ember/string 3.1.1 into dependent project and that causes a lot of issues
- Since we have `capitalize` in @ember/string, we can just call that in our internal helper
- Also bumped internal version of @ember/string to v4.x

Fixes ember-learn#1666
- As per [ember-tether documentation](https://github.com/yapplabs/ember-tether) in testing mode we have to make sure that the portal is attached to correct DOM scope

> Tether works by appending tethered elements to the <body> tag. Unfortunately, this moves your content outside of the Ember application rootElement during acceptance testing. This breaks event dispatch and action handling, including traditional Ember test helpers like click.

This fixes all tests that fail because search popup can't be find.
@MichalBryxi MichalBryxi force-pushed the mb/1666-remove-ember-string branch 2 times, most recently from 9260297 to 3ea95ac Compare May 3, 2025 06:59
@MichalBryxi MichalBryxi changed the title [WIP] fix: Remove ember-cli-string-helpers fix: Remove ember-cli-string-helpers May 3, 2025
@MichalBryxi
Copy link
Contributor Author

Ok so now this PR works. It only does:

  1. Remove ember-cli-string-helpers
  2. Fix ember-tether tests not finding correct container (not sure how it worked before)

Ready for merge @SergeAstapov

@SergeAstapov SergeAstapov merged commit cd2bdc2 into ember-learn:main May 3, 2025
30 checks passed
@MichalBryxi MichalBryxi deleted the mb/1666-remove-ember-string branch May 3, 2025 13:07
@MichalBryxi
Copy link
Contributor Author

Yay \o/

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

Successfully merging this pull request may close these issues.

Remove ember-cli-string-helpers addon
2 participants