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.

Sign up for GitHub

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

Jump to bottom

Add Embroider support #29

Merged
merged 3 commits into from
Jan 11, 2022
Merged

Add Embroider support #29

merged 3 commits into from
Jan 11, 2022

Conversation

mansona
Copy link
Owner

@mansona mansona commented Oct 18, 2021

No description provided.

@mansona mansona changed the title Embroider Add Embroider support Oct 18, 2021
@mansona
Copy link
Owner Author

mansona commented Oct 18, 2021

So it looks like the "embroider safe implementation" is actually working, since all the ember-try scenarios are working.

We seem to be hitting a different problem though 🤔 I'm getting an exception The "path" argument must be of type string. Received undefined which seems to be related to calling path.join() with an undefined value.

I've tracked it down to this line https://github.com/embroider-build/embroider/blob/master/packages/compat/src/compat-addons.ts#L195 where dep.name is undefined. From what I can tell this is the "OwnedAddon" (i.e. the ember-get-config addon itself) but I can't quite see why it doesn't have any of the metadata it needs 🤔 and I really don't know what my next steps should be

Here's a screenshot covering what I said about the current state of the app at that error (see the watch values in the top right):
Screenshot 2021-10-18 at 10 47 54

@mansona mansona mentioned this pull request Oct 20, 2021
Closed
MrChocolatine added a commit to DazzlingFugu/ember-feature-controls that referenced this pull request Nov 1, 2021
@MrChocolatine
build: disable Embroider adapter ember-get-config
b00d82e 
`ember-get-config`, latest version 0.5.0, doesn't support Embroider yet
as this PR has not been merged as the time of writing this commit:

mansona/ember-get-config#29

In parallel, Embroider tests scenarios fail on our side:

https://github.com/peopledoc/ember-feature-controls/actions/runs/1408588177
  |
  `--> Relates to this issue:
       embroider-build/embroider#823

This issue 823 recommends to use `require('@embroider/compat').compatBuild`
in order to disable Embroider's adapter `ember-get-config` (see also
https://github.com/embroider-build/embroider/tree/v0.47.1#options).

But it does not work: `app` is not accepted by `compatBuild()` because
it is an `EmberAddon`, not an `EmberApp`. Log from from the said error:

```
Argument of type 'EmberAddon' is not assignable to parameter of type 'EmberAppInstance'.
  Property 'testIndex' is missing in type 'EmberAddon' but required in type 'EmberAppInstance'.ts(2345)
```

However, the following document indicates we can pass these options to
`maybeEmbroider()` configure Embroider:

https://github.com/embroider-build/embroider/tree/v0.47.1/packages/test-setup#maybeembroiderapp-embroideroptions

References:
- Embroider's adapter for `ember-get-config`:
  https://github.com/embroider-build/embroider/blob/v0.47.1/packages/compat/src/compat-adapters/ember-get-config.ts

- Embroider compatibility options:
  https://github.com/embroider-build/embroider/blob/v0.47.1/packages/compat/src/options.ts#L46-L61

- Discussions on Discord where I first saw the same initial issue:
  - https://discordapp.com/channels/480462759797063690/568935504288940056/901170716949512233
  - https://discordapp.com/channels/480462759797063690/568935504288940056/902484167915364433
MrChocolatine added a commit to DazzlingFugu/ember-feature-controls that referenced this pull request Nov 1, 2021
@MrChocolatine
build: disable Embroider adapter ember-get-config
2fe9186 
`ember-get-config`, latest version 0.5.0, doesn't support Embroider yet
as this PR has not been merged as the time of writing this commit:

mansona/ember-get-config#29

In parallel, Embroider tests scenarios fail on our side:

https://github.com/peopledoc/ember-feature-controls/actions/runs/1408588177
  |
  `--> Relates to this issue:
       embroider-build/embroider#823

This issue 823 recommends to use `require('@embroider/compat').compatBuild`
in order to disable Embroider's adapter `ember-get-config` (see also
https://github.com/embroider-build/embroider/tree/v0.47.1#options).

But it does not work: `app` is not accepted by `compatBuild()` because
it is an `EmberAddon`, not an `EmberApp`. Log from the error:

```
Argument of type 'EmberAddon' is not assignable to parameter of type 'EmberAppInstance'.
  Property 'testIndex' is missing in type 'EmberAddon' but required in type 'EmberAppInstance'.ts(2345)
```

However, the following document indicates we can pass these options to
`maybeEmbroider()` to configure Embroider:

https://github.com/embroider-build/embroider/tree/v0.47.1/packages/test-setup#maybeembroiderapp-embroideroptions

References:
- Embroider's adapter for `ember-get-config`:
  https://github.com/embroider-build/embroider/blob/v0.47.1/packages/compat/src/compat-adapters/ember-get-config.ts

- Embroider compatibility options:
  https://github.com/embroider-build/embroider/blob/v0.47.1/packages/compat/src/options.ts#L46-L61

- Discussions on Discord where I first saw the same initial issue:
  - https://discordapp.com/channels/480462759797063690/568935504288940056/901170716949512233
  - https://discordapp.com/channels/480462759797063690/568935504288940056/902484167915364433
MrChocolatine added a commit to DazzlingFugu/ember-feature-controls that referenced this pull request Nov 2, 2021
@MrChocolatine
build: disable Embroider adapter ember-get-config
9dee31f 
`ember-get-config`, latest version 0.5.0, doesn't support Embroider yet
as this PR has not been merged as the time of writing this commit:

mansona/ember-get-config#29

In parallel, Embroider tests scenarios fail on our side:

https://github.com/peopledoc/ember-feature-controls/actions/runs/1408588177
  |
  `--> Relates to this issue:
       embroider-build/embroider#823

This issue 823 recommends to use `require('@embroider/compat').compatBuild`
in order to disable Embroider's adapter `ember-get-config` (see also
https://github.com/embroider-build/embroider/tree/v0.47.1#options).

But it does not work: `app` is not accepted by `compatBuild()` because
it is an `EmberAddon`, not an `EmberApp`. Log from the error:

```
Argument of type 'EmberAddon' is not assignable to parameter of type 'EmberAppInstance'.
  Property 'testIndex' is missing in type 'EmberAddon' but required in type 'EmberAppInstance'.ts(2345)
```

However, the following document indicates we can pass these options to
`maybeEmbroider()` to configure Embroider:

https://github.com/embroider-build/embroider/tree/v0.47.1/packages/test-setup#maybeembroiderapp-embroideroptions

References:
- Embroider's adapter for `ember-get-config`:
  https://github.com/embroider-build/embroider/blob/v0.47.1/packages/compat/src/compat-adapters/ember-get-config.ts

- Embroider compatibility options:
  https://github.com/embroider-build/embroider/blob/v0.47.1/packages/compat/src/options.ts#L46-L61

- Discussions on Discord where I first saw the same initial issue:
  - https://discordapp.com/channels/480462759797063690/568935504288940056/901170716949512233
  - https://discordapp.com/channels/480462759797063690/568935504288940056/902484167915364433
MrChocolatine added a commit to DazzlingFugu/ember-feature-controls that referenced this pull request Nov 2, 2021
@MrChocolatine
build: disable Embroider adapter ember-get-config
64963e4 
`ember-get-config`, latest version 0.5.0, doesn't support Embroider yet
as this PR has not been merged as the time of writing this commit:

mansona/ember-get-config#29

In parallel, Embroider tests scenarios fail on our side:

https://github.com/peopledoc/ember-feature-controls/actions/runs/1408588177
  |
  `--> Relates to this issue:
       embroider-build/embroider#823

This issue 823 recommends to use `require('@embroider/compat').compatBuild`
in order to disable Embroider's adapter `ember-get-config` (see also
https://github.com/embroider-build/embroider/tree/v0.47.1#options).

But it does not work: `app` is not accepted by `compatBuild()` because
it is an `EmberAddon`, not an `EmberApp`. Log from the error:

```
Argument of type 'EmberAddon' is not assignable to parameter of type 'EmberAppInstance'.
  Property 'testIndex' is missing in type 'EmberAddon' but required in type 'EmberAppInstance'.ts(2345)
```

However, the following document indicates we can pass these options to
`maybeEmbroider()` to configure Embroider:

https://github.com/embroider-build/embroider/tree/v0.47.1/packages/test-setup#maybeembroiderapp-embroideroptions

References:
- Embroider's adapter for `ember-get-config`:
  https://github.com/embroider-build/embroider/blob/v0.47.1/packages/compat/src/compat-adapters/ember-get-config.ts

- Embroider compatibility options:
  https://github.com/embroider-build/embroider/blob/v0.47.1/packages/compat/src/options.ts#L46-L61

- Discussions on Discord where I first saw the same initial issue:
  - https://discordapp.com/channels/480462759797063690/568935504288940056/901170716949512233
  - https://discordapp.com/channels/480462759797063690/568935504288940056/902484167915364433
MrChocolatine added a commit to DazzlingFugu/ember-feature-controls that referenced this pull request Nov 2, 2021
@MrChocolatine
build: disable Embroider adapter ember-get-config
f9da690 
`ember-get-config`, latest version 0.5.0, doesn't support Embroider yet
as this PR has not been merged as the time of writing this commit:

mansona/ember-get-config#29

In parallel, Embroider tests scenarios fail on our side:

https://github.com/peopledoc/ember-feature-controls/actions/runs/1408588177
  |
  `--> Relates to this issue:
       embroider-build/embroider#823

This issue 823 recommends to use `require('@embroider/compat').compatBuild`
in order to disable Embroider's adapter `ember-get-config` (see also
https://github.com/embroider-build/embroider/tree/v0.47.1#options).

But it does not work: `app` is not accepted by `compatBuild()` because
it is an `EmberAddon`, not an `EmberApp`. Log from the error:

```
Argument of type 'EmberAddon' is not assignable to parameter of type 'EmberAppInstance'.
  Property 'testIndex' is missing in type 'EmberAddon' but required in type 'EmberAppInstance'.ts(2345)
```

However, the following document indicates we can pass these options to
`maybeEmbroider()` to configure Embroider:

https://github.com/embroider-build/embroider/tree/v0.47.1/packages/test-setup#maybeembroiderapp-embroideroptions

References:
- Embroider's adapter for `ember-get-config`:
  https://github.com/embroider-build/embroider/blob/v0.47.1/packages/compat/src/compat-adapters/ember-get-config.ts

- Embroider compatibility options:
  https://github.com/embroider-build/embroider/blob/v0.47.1/packages/compat/src/options.ts#L46-L61

- Discussions on Discord where I first saw the same initial issue:
  - https://discordapp.com/channels/480462759797063690/568935504288940056/901170716949512233
  - https://discordapp.com/channels/480462759797063690/568935504288940056/902484167915364433
MrChocolatine added a commit to DazzlingFugu/ember-feature-controls that referenced this pull request Nov 2, 2021
@MrChocolatine
build: disable Embroider adapter ember-get-config
28eba81 
`ember-get-config`, latest version 0.5.0, doesn't support Embroider yet
as this PR has not been merged as the time of writing this commit:

mansona/ember-get-config#29

In parallel, Embroider tests scenarios fail on our side:

https://github.com/peopledoc/ember-feature-controls/actions/runs/1408588177
  |
  `--> Relates to this issue:
       embroider-build/embroider#823

This issue 823 recommends to use `require('@embroider/compat').compatBuild`
in order to disable Embroider's adapter `ember-get-config` (see also
https://github.com/embroider-build/embroider/tree/v0.47.1#options).

But it does not work: `app` is not accepted by `compatBuild()` because
it is an `EmberAddon`, not an `EmberApp`. Log from the error:

```
Argument of type 'EmberAddon' is not assignable to parameter of type 'EmberAppInstance'.
  Property 'testIndex' is missing in type 'EmberAddon' but required in type 'EmberAppInstance'.ts(2345)
```

However, the following document indicates we can pass these options to
`maybeEmbroider()` to configure Embroider:

https://github.com/embroider-build/embroider/tree/v0.47.1/packages/test-setup#maybeembroiderapp-embroideroptions

References:
- Embroider's adapter for `ember-get-config`:
  https://github.com/embroider-build/embroider/blob/v0.47.1/packages/compat/src/compat-adapters/ember-get-config.ts

- Embroider compatibility options:
  https://github.com/embroider-build/embroider/blob/v0.47.1/packages/compat/src/options.ts#L46-L61

- Discussions on Discord where I first saw the same initial issue:
  - https://discordapp.com/channels/480462759797063690/568935504288940056/901170716949512233
  - https://discordapp.com/channels/480462759797063690/568935504288940056/902484167915364433
@jrjohnson jrjohnson mentioned this pull request Dec 16, 2021
Closed
@NullVoxPopuli
Copy link

@mansona do you have an open issue on the embroider repo?
Wondering what needs to be done, todos, etc
Trying to get off my custom branch of ember-fontawesome 😅

@davideferre
Copy link

@mansona I think you only need to update the @embroider/test-setup version from 0.43.5 to 0.49.0 in your package.json and all the two embroider ember-try scenarios will succeed.
As @NullVoxPopuli says this problem is currently blocking the ember-fontawesome release that supports ember 4 😅.

@mansona
Copy link
Owner Author

mansona commented Jan 11, 2022

@davideferre thanks that has fixed it! I'd be super curious what the issue was upstream 🙃

@mansona mansona merged commit 90be24e into master Jan 11, 2022
@delete-merged-branch delete-merged-branch bot deleted the embroider branch January 11, 2022 14:25
@mansona
Copy link
Owner Author

mansona commented Jan 11, 2022

Also I'm curious why you said that this was blocking the Ember 4 release? Embroider and Ember 4 are 2 different issues 🤔

@ef4
Copy link

ef4 commented Jan 11, 2022

Be advised that embroider 0.48.0 and higher have a working compat adapter that completely overrides ember-get-config's implementation anyway: https://github.com/embroider-build/embroider/blob/f7a4ad0e4d939446ba47c5a0a1adb4a4bb463b9d/packages/compat/src/compat-adapters/ember-get-config.ts

Which is why apps are able to use embroider even when some of their addons have a version of ember-get-config that doesn't work.

We can update the compat adapter so it leaves ember-get-config 1.0 alone. And pretty soon we can port ember-get-config to v2 format, which avoids the whole compat system. I think that is blocked on implementing a part of the v2 addon spec that we haven't had to do yet (the build file support).

@davideferre
Copy link

Also I'm curious why you said that this was blocking the Ember 4 release? Embroider and Ember 4 are 2 different issues thinking

Sorry, I don't know why I wrote Ember 4 instead of Embroider, maybe the late hour can explains this error 😅

MrChocolatine added a commit to DazzlingFugu/ember-feature-controls that referenced this pull request Nov 4, 2023
@MrChocolatine
build: rm compatAdapters override for ember-get-config
c2cb36a 
1. The coment says this override can be removed once mansona/ember-get-config#29
   has been merged, and it was merged.
2. `ember-get-config` package was uninstalled in commit 56b2fe8 .
MrChocolatine added a commit to DazzlingFugu/ember-feature-controls that referenced this pull request Nov 4, 2023
@MrChocolatine
build: rm compatAdapters override for ember-get-config
7746d10 
1. The coment says this override can be removed once mansona/ember-get-config#29
   has been merged, and it was merged.
2. `ember-get-config` package was uninstalled in commit 56b2fe8 .
MrChocolatine added a commit to DazzlingFugu/ember-feature-controls that referenced this pull request Nov 4, 2023
@MrChocolatine
build: rm compatAdapters override for ember-get-config
c38734f 
1. The coment says this override can be removed once mansona/ember-get-config#29
   has been merged, and it was merged.
2. `ember-get-config` package was uninstalled in commit 56b2fe8 .
MrChocolatine added a commit to DazzlingFugu/ember-feature-controls that referenced this pull request Nov 4, 2023
@MrChocolatine
build: rm compatAdapters override for ember-get-config
e34af43 
1. The coment says this override can be removed once mansona/ember-get-config#29
   has been merged, and it was merged.
2. `ember-get-config` package was uninstalled in commit 56b2fe8 .
MrChocolatine added a commit to DazzlingFugu/ember-feature-controls that referenced this pull request Nov 5, 2023
@MrChocolatine
build: rm compatAdapters override for ember-get-config
7cc7a01 
1. The coment says this override can be removed once mansona/ember-get-config#29
   has been merged, and it was merged.
2. `ember-get-config` package was uninstalled in commit 56b2fe8 .
MrChocolatine added a commit to DazzlingFugu/ember-feature-controls that referenced this pull request Nov 11, 2023
@MrChocolatine
build: rm compatAdapters override for ember-get-config
89bc33a 
1. The coment says this override can be removed once mansona/ember-get-config#29
   has been merged, and it was merged.
2. `ember-get-config` package was uninstalled in commit 56b2fe8 .
MrChocolatine added a commit to DazzlingFugu/ember-feature-controls that referenced this pull request Nov 12, 2023
@MrChocolatine
build: rm compatAdapters override for ember-get-config
c2a3b8b 
1. The coment says this override can be removed once mansona/ember-get-config#29
   has been merged, and it was merged.
2. `ember-get-config` package was uninstalled in commit 56b2fe8 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mansona @NullVoxPopuli @davideferre @ef4