appuniversum / ember-appuniversum

Ember addon wrapping the appuniversum components.
https://appuniversum.github.io/ember-appuniversum
MIT License
14 stars 11 forks source link

Use the icon components internally #486

Closed Windvis closed 5 months ago

Windvis commented 5 months ago

This is a good experiment to see if everything works, and it allows projects to skip the svg-symbolset download if they also refactor their own code.

Builds on #483 and is part of #482

Windvis commented 5 months ago

@piemonkey (CC @abeforgit) this should cover everything to unblock the embeddable I think.

I'm not familiar with the project, but maybe it might be interesting to yalc this PR and try it out in the app? Or do you prefer merging and releasing as an experimental feature?

piemonkey commented 5 months ago

@abeforgit we need to be using https to test this fully, right?

I have a local branch of embeddable that I used to test the component passing PR, so I can update this to use this branch as a git dep. Then I can test it locally with a proxy and a self-signed cert. I think that's the easiest, though if others want to test they'll also need to do the https set-up.

Windvis commented 5 months ago

I didn't realise that you'd gone for the build-on-package approach in the end.

Yea, it didn't feel right and I consider this a temporary setup anyways since the script will be converted to a rollup plugin once we switch to the v2 addon setup.

I did have to use yalc in the end due to the combination of npm lifecycle scripts and git: dependencies being broken.

While looking into this last week I came across a comment from an npm maintainer who seemed to imply that he considers using github dependencies a very bad practice, so that might explain why things like this don't get fixed 😄.

After updating our use of components to pass icon components instead of strings, this version does indeed fix the problem.

Thanks for testing 🙏. I'll merge and tag a release today.

Windvis commented 5 months ago

I did have to use yalc in the end due to the combination of npm lifecycle scripts and git: dependencies being broken.

Wait, was the issue related to missing dependencies? It seems npm only installs devDeps for git dependencies if there is a prepare script. We're using prepack now (someone in the Discord recommended this) so that might explain it.

NOTE: If a package being installed through git contains a prepare script, its dependencies and devDependencies will be installed, and the prepare script will be run, before the package is packaged and installed.

In any case, I'm not sure if we should actually support using Appuniversum as a Git dependency to be honest, so I'll just leave it as-is for now.

piemonkey commented 5 months ago

I did have to use yalc in the end due to the combination of npm lifecycle scripts and git: dependencies being broken.

Wait, was the issue related to missing dependencies? It seems npm only installs devDeps for git dependencies if there is a prepare script. We're using prepack now (someone in the Discord recommended this) so that might explain it.

I forget for certain now, but I think it was not generating the components at all. I don't think it's a thing that needs to be fixed as I think trying to find a solution that works for that and in all the other possible configurations is either impossible or very hard. I just mentioned it to potentially avoid others wasting time with it in the future.

abeforgit commented 5 months ago

chiming in with: npm git dependencies -> "just don't" edit: and a big thanks for your help of course :)