-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: export svgs from ui for mobile #357
Conversation
49fbedd
to
0292b75
Compare
0292b75
to
2e7d1a5
Compare
I am going to close this bc I got it working for mobile. I need to do a bit more work/refactoring to share the svgs b/w web and mobile. I just wanted to get this first step working before moving fwd. |
@@ -4,12 +4,12 @@ | |||
"license": "MIT", | |||
"type": "module", | |||
"scripts": { | |||
"assets:native": "mkdir -p dist-native/src && cp -r src/assets-native ./dist-native/src/", | |||
"prebuild:native": "mkdir -p dist-native/src && cp -r src/assets-native ./dist-native/src/ && cp index.d.ts ./dist-native/src/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fbwoolf wonder if we can move this asset moving from package.json to the build tool itself, be in metro or tsup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't working for me using either until I added this. There seems to be issues (listed in description) open with tsup and loading svgs. The svgr plugin hits errors but I can try again for mobile. We aren't currently using tsup to build for native. Right now, I'm running into issue using tsup for the web build. I'm also unclear as to why we have a dist-all
and a dist-web
?
We are using the metro config in the UI lib, but I'm unclear how/if it is used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edgarkhanzadian set this up. I'm not sure if we need dist-all
or if this is required for publication purposes.
Either way, wouldn't concern yourself with current set up. Delete it and start again if this helps you solve problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edgarkhanzadian any reason we need to use metro for native bundling? Couldn't we use tsup for this as well, and metro in mobile will handle RN specific bundling concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dist-all
is being used as the package export for web atm. I'm actually not sure if dist-web
is being used or can just replace dist-all
? I'm currently working through changes for web to get the svg export working for the extension, so @edgarkhanzadian any feedback you can provide will be helpful. I'm making changes but don't want to overlook issues you've already confronted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used dist-all before to just be able to compile all of the files in one folder, but i think it's pretty much redundant now. We just need 2 separate build folders for web and mobile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kyranjamie yea i think that's how it's setup in ui library now, we don't use metro there. It's only setup for the mobile storybook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyone know why the index.d.ts
wouldn't be compiled with tsup if in the tsconfig includes
array? I had to add it to the script or it was always missing and the svg type couldn't be resolved in the consuming package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kyranjamie it appears there is a slightly problematic lib being used in tsup that is used to gen types when dts
is true:
egoist/tsup#993
Swatinem/rollup-plugin-dts#277
In docs, there is an experimental option I am going to try using:
https://tsup.egoist.dev/#generate-declaration-file
I tried to put together a simple example of what should work to export svgs from the ui library to consume in mobile, but the svg path isn't resolved when running the mobile app. Am I missing something simple here with the tsconfig for native? I added the
index.d.ts
directly to theincludes
array, but I don't see the types being included. Is that why we have a separateglobal.d.ts
in a types folder? 🤔Questions I have:
I also attempted to build the native code with tsup using the svgr plugin with the
native
flag set to true, but I ran into open discussions/issues:egoist/tsup#811
egoist/tsup#570
I tried using
@svgr/cli
to auto gen icons just to confirm svgr was working, but ran into issues with our prettier config ...so maybe that is why the tsup plugin wasn't ultimately working:https://react-svgr.com/docs/cli/