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

Embroider optimized support #964

Merged
merged 4 commits into from
Sep 7, 2023
Merged

Embroider optimized support #964

merged 4 commits into from
Sep 7, 2023

Conversation

Windvis
Copy link
Contributor

@Windvis Windvis commented Sep 6, 2023

Overview

This should fix the remaining issues in #945 and make the addon fully Embroider "optimized" compatible.

connected issues and PRs:

Closes #945

Setup

How to test/reproduce

Run the embroider optimized scenario: npx ember-cli try:one embroider-optimized and the build should pass and the tests should succeed.

If you want to run the dummy app you could run npx ember-cli try:one embroider-optimized --skip-cleanup instead and run EMBROIDER_TEST_SETUP_OPTIONS=optimized npm start afterwards.

Challenges/uncertainties

Checks PR readiness

  • UI: works on smaller screen sizes
  • UI: feedback for any loading/error states
  • Check cancel/go-back flows
  • Check database state correct when deleting/updating (especially regarding relationships)
  • changelog
  • npm lint
  • no new deprecations

@Windvis Windvis added the breaking Breaking api changes label Sep 6, 2023
@Windvis Windvis changed the title Embroider optimized fixes Embroider optimized support Sep 6, 2023
@Windvis Windvis force-pushed the embroider-optimized-fixes branch 2 times, most recently from 042d14c to aa569af Compare September 6, 2023 09:30
import { Node as PNode } from 'prosemirror-model';

const emberNodeConfig: EmberNodeConfig = {
name: 'image',
componentPath: 'plugins/image/node',
component: Image as unknown as ComponentLike,
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 seems there are type conflicts between our components and the ComponentLike type from @glint/template. Not sure what is up exactly, maybe outdated type packages. More investigation is needed. I used this workaround for now since TS isn't my forte 😬.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I read through this issue and it does sound like we are doing the right thing with using ComponentLike<any> but I might be misunderstanding things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I also asked in the discord, but haven't received a response yet.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the @glint/template package only added for the ComponentLike type?

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be possible that the types included with ember 5.0 are compatible with the glint types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the @glint/template package only added for the ComponentLike type?

Yes, that seemed to be the recommended way to type use-cases like that.

It could be possible that the types included with ember 5.0 are compatible with the glint types?

Yea, possibly, I haven't tried, but those are also not available on older versions so I don't think we can use them yet?

@Windvis Windvis force-pushed the embroider-optimized-fixes branch 2 times, most recently from c5f529d to 053d6ae Compare September 6, 2023 10:45
@Windvis
Copy link
Contributor Author

Windvis commented Sep 6, 2023

Should I enable the embroider-optimized scenario here? Or do we keep that as a local only thing to reduce CI load?

ember-velcro uses the "nested" component structure where an index.js are located in the `app/components/velcro/` folder. This no longer seems to work for v2 addons.

As a workaround we can import the component and invoke it directly. This is also a transition path to .gts files.
The component was renamed a while ago but there were still some remnants around that caused issues under Embroider.
This replaces the `componentPath` with a `component` argument. Users need to provide the component class instead of the path which makes the setup Embroider compatible.
@abeforgit
Copy link
Member

We're still figuring out how we're gonna manage the ember-try scenarios with woodpecker, so leave it off for now

Copy link
Contributor

@elpoelma elpoelma left a comment

Choose a reason for hiding this comment

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

`Everything seems to work nicely, the embroider-optimized scenario succeeds. The API changes to ember-nodes are sensible :)

@abeforgit abeforgit merged commit 2d8e36d into master Sep 7, 2023
@abeforgit abeforgit deleted the embroider-optimized-fixes branch September 7, 2023 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking api changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Embroider compatibility
3 participants