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

Use the icon components internally #486

Merged
merged 1 commit into from
Mar 25, 2024

Conversation

Windvis
Copy link
Contributor

@Windvis Windvis commented Mar 21, 2024

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
Copy link
Contributor Author

Windvis commented Mar 21, 2024

@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?

@Windvis Windvis added the internal Used for internal changes that still require a mention in the changelog/release notes. label Mar 21, 2024
@piemonkey
Copy link
Contributor

@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.

Copy link
Contributor

@piemonkey piemonkey left a comment

Choose a reason for hiding this comment

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

I did have to use yalc in the end due to the combination of npm lifecycle scripts and git: dependencies being broken. I didn't realise that you'd gone for the build-on-package approach in the end. I didn't need the https proxy though, the problem is evident without it.

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

@Windvis
Copy link
Contributor Author

Windvis commented Mar 25, 2024

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
Copy link
Contributor Author

Windvis commented Mar 25, 2024

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.

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.
@Windvis Windvis force-pushed the refactor/use-icon-components-internally branch from b31ffe6 to 5798602 Compare March 25, 2024 11:56
@Windvis Windvis mentioned this pull request Mar 25, 2024
6 tasks
@Windvis Windvis merged commit d56366a into master Mar 25, 2024
8 checks passed
@Windvis Windvis deleted the refactor/use-icon-components-internally branch March 25, 2024 12:07
@piemonkey
Copy link
Contributor

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
Copy link
Contributor

abeforgit commented Mar 27, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Used for internal changes that still require a mention in the changelog/release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants