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

Ember: Remove global Ember usage #17843

Merged
merged 5 commits into from Jul 3, 2022
Merged

Ember: Remove global Ember usage #17843

merged 5 commits into from Jul 3, 2022

Conversation

dbendaou
Copy link
Member

@dbendaou dbendaou commented Mar 31, 2022

Issue:

What I did

Remove Ember global usage in favor of Component directly

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Mar 31, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit e2fda25. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@yannbf
Copy link
Member

yannbf commented Apr 11, 2022

Hey @dbendaou thanks for this contribution! Is there a reason this PR is still a draft? Thanks!

@yannbf yannbf added the ember label Apr 11, 2022
@dbendaou dbendaou marked this pull request as ready for review April 14, 2022 07:54
@dbendaou
Copy link
Member Author

Updated!

@dbendaou dbendaou added the maintenance User-facing maintenance tasks label May 4, 2022
@dbendaou
Copy link
Member Author

dbendaou commented May 4, 2022

Could we move forward and have this merged? Since for now Storybook is unusable with Ember4 app

Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

@shilman could you release this? Thanks ♥️

@krezicoder
Copy link

Hi @shilman when can we expect this to be released ? Our entire dev workflow is blocked as we are not able to use storybook with ember 4.x

@shilman shilman changed the title ♻️ Remove global Ember. usage and rework with component import Ember: Remove global Ember usage May 10, 2022
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

@dbendaou @yannbf @krezicoder This PR breaks CI -- specifically, it looks like there's a missing dependency on @ember/component:

Cursor_and_Mouse_Highlight_Overlay

@dbendaou
Copy link
Member Author

dbendaou commented May 31, 2022

Hey @shilman, I've updated my PR and updated the ember source dependencies to be able to leverage the module unification introduced in 3.27+ (previously it wasn't using "real" module)
It seems that TS cannot resolve the @ember/component dependencies, however, it's correctly implemented in ember-source here

And I'm able to properly run the Ember example
Screenshot 2022-05-31 at 16 46 22

Also, ember-source doesn't provide type definition yet, they are working on it..

Do you know if I'm missing something ?

@dbendaou dbendaou requested a review from shilman June 1, 2022 09:08
@gossi
Copy link
Contributor

gossi commented Jun 2, 2022

You may be able to pull in @types/ember__component to calm the CI for typechecks

@ndelangen
Copy link
Member

How could this possibly work?
This @ember/component thing doesn't actually exist at all, on npm

@gossi
Copy link
Contributor

gossi commented Jul 2, 2022

@ember/component (amongst others) is a virtual package. ember-cli magic from the dark days makes it available during serving ember apps (it is virtual until this can be published on npm directly). Given this code is ~10yrs old, the virtual package is the required migration step.

What's needed to move this forward?

@ndelangen
Copy link
Member

@gossi If you think this is good to merge, that's all that's required from me!

Please merge!, I think you have the access-level to do that, don't you?

@gossi
Copy link
Contributor

gossi commented Jul 3, 2022

Merging is blocked for me, @shilman requested changes. Can you unblock me or merge directly?

@ndelangen ndelangen merged commit b4bf826 into next Jul 3, 2022
@ndelangen ndelangen deleted the remove-ember-usage branch July 3, 2022 09:25
@ndelangen
Copy link
Member

Merged!! Thank you @dbendaou and @gossi

@gossi
Copy link
Contributor

gossi commented Nov 30, 2022

@dbendaou we should backport this to storybook v6 - as it will take more time to make storybook v7 compatible with recent ember developments.

@SergeAstapov
Copy link

@shilman @dbendaou @gossi @ndelangen I'm open to help cherry-pick these changes to v6 but little confused which branch it should target. should it be next-release?

@shilman
Copy link
Member

shilman commented Dec 12, 2022

@SergeAstapov we’ll try to take care of it and will let you know if we need help cherry picking

@shilman shilman added patch:yes Bugfix & documentation PR that need to be picked to main branch patch:done Patch/release PRs already cherry-picked to main/release branch labels Dec 12, 2022
@shilman
Copy link
Member

shilman commented Dec 12, 2022

This is now available in 6.5.15-alpha.0

@dbendaou
Copy link
Member Author

dbendaou commented Jan 3, 2023

We didn't had any issue and it's currently working ok in the alpha, do you think we could add it in the next 6.x release?

@SergeAstapov
Copy link

couple minor issues I've noticed with `peerDependencies:

    "@types/ember__component": "4.0.8",
    "ember-source": "~3.28.1"

this causes some issues... I believe these have to be changed to

    "@types/ember__component": "^4.0.8",
    "ember-source": "~3.28.1 || ^4.0.0"

@ndelangen
Copy link
Member

@SergeAstapov Would you be open to opening a PR to remedy this? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ember maintenance User-facing maintenance tasks patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants