jxnblk / microicon

SVG icon microservice
https://icon.now.sh
359 stars 25 forks source link

Set Cache-Control headers to 1hr #14

Closed martinklepsch closed 5 years ago

martinklepsch commented 5 years ago

Fixes #5

Set's the Cache-Control header to 3600s / 1hr. This should improve performance for icon delivery by allowing Now's CDN to cache responses for up to one hour.

Potentially the max-age setting could be more aggressive since the data doesn't really change all that much? 3hrs? 12hrs? 3days?

I wasn't able to test this on now.sh yet (getting the error below when running now, see deployment) but the change is fairly straightforward so maybe that's ok? 😄

> Verifying instantiation in bru1
> [0]
> [0] microicon@1.0.0-b.2 start /home/nowuser/src
> [0] micro -p 3000 server/index.js
> [0]
> [0] micro: Error when importing /home/nowuser/src/server/index.js: Error: Cannot find module '..'
> [0]     at Function.Module._resolveFilename (module.js:547:15)
> [0]     at Function.Module._load (module.js:474:25)
> [0]     at Module.require (module.js:596:17)
> [0]     at require (internal/module.js:11:18)
> [0]     at Object.<anonymous> (/home/nowuser/src/server/index.js:8:19)
> [0]     at Module._compile (module.js:652:30)
> [0]     at Object.Module._extensions..js (module.js:663:10)
> [0]     at Module.load (module.js:565:32)
> [0]     at tryModuleLoad (module.js:505:12)
> [0]     at Function.Module._load (module.js:497:3)
> [0]     at Module.require (module.js:596:17)
> [0]     at require (internal/module.js:11:18)
> [0]     at module.exports.file (/home/nowuser/src/node_modules/micro/lib/module.js:6:11)
> [0]     at module.exports (/home/nowuser/src/node_modules/micro/lib/index.js:9:47)
> [0]     at Object.<anonymous> (/home/nowuser/src/node_modules/micro/bin/micro.js:90:18)
> [0]     at Module._compile (module.js:652:30)
martinklepsch commented 5 years ago

Not entirely sure if what I did is correct but it seemed the repo was in a slightly broken state.

There's a running deployment of this here: https://microicon-yintbnpbdc.now.sh

I think we could still increase the max-age to maybe 12 or 24 hours since the assets are really mostly static but not sure if you have any preferences in the regard.

Happy to drop individual commits, rebase, squash or whatever else you'd like.

jxnblk commented 5 years ago

Thanks! Yeah, I haven’t touched this in a while so it doesn’t surprise me that simple icons went out of sync; it wasn’t on npm when I put this up. I think the caching could be more aggressive like you suggested and I can also get automatic now deploys set up on another branch if that’s helpful

martinklepsch commented 5 years ago

I figured out the deployment issues so the branch deploys aren't necessary (for me).

it wasn’t on npm when I put this up

I wasn't aware it's on npm. The changes I made still use napa to copy it into node_modules. Would you prefer it to use the npm package?

I think the caching could be more aggressive like you suggested

Should we set max-age to 24hrs and ship this?

jxnblk commented 5 years ago

It looks like it's published now, but haven't tested it out: https://www.npmjs.com/package/simple-icons

martinklepsch commented 5 years ago

Yeah I found that package as well... I’ll take a look at removing the napa stuff and replacing it with the npm package. 👍

martinklepsch commented 5 years ago

@jxnblk Added another commit that removes napa, available under https://microicon-cmpilmdzgj.now.sh.

Let me know if anything else needs to be done before this can get merged 👍

jxnblk commented 5 years ago

Thanks for this! Going to merge this in and set up automatic deploys on another branch

martinklepsch commented 5 years ago

@jxnblk Hey Brent, just wanted to check in if you had a chance to deploy this? I'm still seeing requests being returned without a Cache-Control header.

martinklepsch commented 5 years ago

Ping 🙂

jxnblk commented 5 years ago

I tried to deploy the updates, but something is causing the build to fail on Now. Haven’t had much time to dig in

martinklepsch commented 5 years ago

Could you provide more details in what way the build is failing?

EDIT I was able to deploy from master just fine: https://microicon-cmpilmdzgj.now.sh

martinklepsch commented 5 years ago

BTW, I looked into using Now 2.0 but there are currently some limitations with regards to the size of node_modules and the bundle in general. material-design-icons is unfortunately exceeding the limits with its 356M.

https://spectrum.chat/thread/954196ab-a020-46e8-ad33-f10595b9878d

martinklepsch commented 5 years ago

Hey Brent, sorry for bothering you again but have you had a chance to try deploying to Now again or could share some more details on the problems you encountered during deployment?

martinklepsch commented 5 years ago

Hey Brent, sorry for pinging again, just wanted to see if there's anything I could do to help get this into production. I don't have much experience with Now but maybe you can also create a team for this project and add me to it so I could look into the deployment issues you described.

Cheers

martinklepsch commented 5 years ago

Bump 🙂