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

feat(ipx): support nuxt layers #1177

Merged
merged 22 commits into from
Jan 17, 2024
Merged

Conversation

Aareksio
Copy link
Contributor

@Aareksio Aareksio commented Jan 4, 2024

Issue: #940

The issue is caused by IPX receiving only the root public directory. IPX's node-fs storage does not support multiple directories. I've migrated to unstorage (as per TODO left in comment. Next I implemented combine driver which combines multiple unstorage storages, at least the parts which gets used by IPX.


Considerations

  1. The performance of combine driver is probably a bit worse than if it was all single storage, but I do not think this is a big issue considering it gets enabled only in dev, when using layers with public assets. And even then, I do not think any degradation would be noticeable with sane number of images.

  2. Please notice unstorage added to devDependencies. I am unsure where it belongs. ipx is devDependency and is marked as external in build.config.ts. But I never had to install ipx separately when adding @nuxt/image, not quite sure how it works.

  3. IPXRuntimeConfig has changed. Please check if it makes sense. The is only one fs-lite driver option that could be exposed (ignore). As the directory is supposed to be deployed (public), I do not think there is a point in allowing @nuxt/image user configuring underlying storage solution. Ignoring should be either dealt on module level or IPX. Not in provider configuration.

  4. I find the image.dir module option weird in context of ipx code. In module.ts we resolve it to absolute path, but later in ipx.ts we resolve it again...? This also makes it impossible to resolve against layers. Docs have 3 notes explaining contexts the option does not work. Maybe it is a good idea to remove it and just recommend people to stick to public?


@pi0 I could not figure out how to use unstorageToIPXStorage from unstorage. It requires prefix and does not translate / to : in path, making it unusable. It is probably a good idea to update unstorage with proper implementation and remove the makeshift one from here. Also a proper built-in combine utility, or node-fs-multiple driver would have helped a lot.

@danielroe danielroe requested a review from pi0 January 5, 2024 15:47
src/runtime/ipx.ts Outdated Show resolved Hide resolved
@Aareksio
Copy link
Contributor Author

@pi0 Migratated the PR to use unjs/ipx#203. Tested locally with ipx@2.1.0, I did not want to update deps as part of feature, assuming they gonna be bumped independently. I've also tried to mimic the style from your commits on my other PRs, minimal diff, helper functions after main one, do not break logic if not necessary. Hope it fits your vision :)

Also kept the TODO comment, since we reverted back to ipx's fs driver.

@pi0
Copy link
Member

pi0 commented Jan 15, 2024

Amazing thanks for efforts. Will double check in the meantime feel free to bump dep in same PR should be fine.

@codecov-commenter
Copy link

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (05621b8) 68.90% compared to head (81359d9) 69.01%.

Files Patch % Lines
src/ipx.ts 82.60% 4 Missing ⚠️
src/runtime/ipx.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1177      +/-   ##
==========================================
+ Coverage   68.90%   69.01%   +0.10%     
==========================================
  Files          75       75              
  Lines        4245     4266      +21     
  Branches      393      399       +6     
==========================================
+ Hits         2925     2944      +19     
- Misses       1293     1295       +2     
  Partials       27       27              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

Amazing ❤️ 🔥

@danielroe danielroe requested a review from pi0 January 17, 2024 11:46
@Aareksio
Copy link
Contributor Author

Sorry for not pushing the update myself, got caught in other work 😅

@danielroe
Copy link
Member

Not to worry! This is a great improvement. ❤️

src/ipx.ts Outdated Show resolved Hide resolved
src/ipx.ts Outdated Show resolved Hide resolved
Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Other than two above comments looks nice thanks for helping on this ❤️

@danielroe danielroe merged commit 42bbf73 into nuxt:main Jan 17, 2024
2 checks passed
imphil added a commit to fossi-foundation/fossi-foundation-web that referenced this pull request Jan 31, 2024
* Pin to nuxt/image 1.2.0, since 1.3.0 breaks IPX rendering.
  Most likely due to nuxt/image#1177.
* Add an explicit resolution for jackspeak to work around
  nuxt/nuxt#21231.
imphil added a commit to fossi-foundation/fossi-foundation-web that referenced this pull request Jan 31, 2024
* Pin to nuxt/image 1.2.0, since 1.3.0 breaks IPX rendering.
  Most likely due to nuxt/image#1177.
* Add an explicit resolution for jackspeak to work around
  nuxt/nuxt#21231.
riddla pushed a commit to tricks-gmbh/nuxt-image that referenced this pull request Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants