Closed dnlsndr closed 2 years ago
Seems like there is still an issue i need to fix. Damn, it's not as easy as it seems. The library is now compiled to esmodules, but it still seems as if it's not tree-shakable. Will have to do some more research.
For anybody interested, here's a good resource on what's the issue at hand (mainly compiling with rollup, and tree-shaking with webpack as already stated in a different issue): https://blog.theodo.com/2021/04/library-tree-shaking/
@rektdeckard I've fixed some final issues here. I created a package of this repo to test it in our local setups under @dnlsndr/vue-phosphor-icons
and it works pretty well.
Thanks for putting the work in @dnlsndr. Please run the formatting script to eliminate all these single quotes and properly indent all the generated code. I'll give this a test later tonight!
For future ref, you can use the example
demo app to test the bundled lib in a demo app immediately, or npm link
command to test in an existing local project, without having to gum up the npm public registry with random test modules.
@rektdeckard Yeah sorry about that. I do have an issue with using the example app, because it only uses the source files instead of testing the production output, so it's not really representative of what you may get in the end after building the library, especially when testing for tree-shakability. One could of courae just do a npm pack
or as you mentioned npm link
for testing but one wouldn't want to gunk up the repo with yet another special configuration in the package.json
or is it possible with npm link
without mutating the repo for that special testing case? (I'm really not all to versed in npm package deployment). I'm also not in favor of needlessly publishing packages to npm
, but since we ourselves also needed a quick fix, I had to publish the fixed version of the package anyways for our deployments to reference. Maybe one could add a CONTRIBUTING
guide so that anybody who wants to fix something with the library knows exactly what they need to do (including running the format script). I'd be glad to help out with this repo in any way possible, since as I already mentioned, we do also heavily rely on Phosphor icons at work and thus also want to ensure that the library meets general standards.
I've reverted several needless formatting changes.
I think it should be ready for review now
Taking a look now!
nit: keep commit titles to 80 chars or move rest to description
Works a charm, thanks @dnlsndr
@dnlsndr Actually, reverting because it's incorrect to export the component types as a .ts
file. We should emit ambient declarations in a .d.ts
file using export declare
syntax, in the way that Definitely Typed would do.
How we do it in the React lib: https://unpkg.com/browse/phosphor-react@1.4.0/dist/index.d.ts
I'll have a look
This commit add tree-shakability so that one can import only the icons that one needs. Due to some other improvements and general standards, the project structure has changed a tiny bit.
fixes #15