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: add directus provider #787

Merged
merged 26 commits into from
Jun 6, 2023
Merged

feat: add directus provider #787

merged 26 commits into from
Jun 6, 2023

Conversation

sandros94
Copy link
Contributor

Lately I started experimenting with Directus Headless CMS and to my surprise it has a nice implementation and usage of sharp.

So I decided to add it as a provider for ease of use.

@codesandbox
Copy link

codesandbox bot commented Mar 29, 2023

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@netlify
Copy link

netlify bot commented Mar 29, 2023

Deploy Preview for nuxt-image-v1 ready!

Name Link
🔨 Latest commit da75443
🔍 Latest deploy log https://app.netlify.com/sites/nuxt-image-v1/deploys/6479ab8d1145020008411269
😎 Deploy Preview https://deploy-preview-787--nuxt-image-v1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@nuxt-studio
Copy link

nuxt-studio bot commented Mar 29, 2023

Live Preview ready!

Name Edit Preview Latest Commit
Image Edit on Studio ↗︎ View Live Preview da75443

@sandros94
Copy link
Contributor Author

✅ Live Preview ready!

Name Edit Preview Latest Commit
Image Edit on Studio ↗︎ View Live Preview b5edae4

I really can't wait to test nuxt.studio 🥲

@pi0 pi0 changed the title feat(provider): directus feat: add directus provider Mar 31, 2023
@pi0 pi0 added enhancement New feature or request provider labels Mar 31, 2023
@genu
Copy link

genu commented May 25, 2023

LGTM

@danielroe
Copy link
Member

Would you be able to merge the latest main and add snapshots/expected values for directus? 🙏

Copy link
Contributor Author

Would you be able to merge the latest main and add snapshots/expected values for directus?

I've yet never used/created vitest, but there is always a first time right?
I've looked into the commit you did about the tests. Since fully testing directus provider should need a custom test like strapi (with a full directus+db deploy), I should go for the expected values right?

I should add the entry inside the e2e/__snapshots__/*ssr.test.ts.snap, add to provider-coverage.test.ts:

const missingProviderTests = [
  'directus', // covered in a unique test
  'strapi', // covered in a unique test
  'layer0' // backwards-compatible alias for edgio
]

and what else?

@danielroe
Copy link
Member

You could add entries for directus in this file:

image/test/providers.ts

Lines 2 to 23 in 28f550d

{
args: ['/test.png', {}],
ipx: { url: '/_ipx/_/test.png' },
cloudflare: { url: '/test.png' },
cloudinary: { url: '/f_auto,q_auto/test' },
twicpics: { url: '/test.png' },
fastly: { url: '/test.png' },
glide: { url: '/test.png' },
gumlet: { url: '/test.png' },
imgix: { url: '/test.png' },
imageengine: { url: '/test.png' },
unsplash: { url: '/test.png' },
imagekit: { url: '/test.png' },
netlify: { url: '/test.png' },
prismic: { url: '/test.png?auto=compress,format&rect=0,0,200,200&w=100&h=100' },
sanity: { url: 'https://cdn.sanity.io/images/projectid/production/test-300x450.png?auto=format' },
contentful: { url: '/test.png' },
cloudimage: { url: 'https://demo.cloudimg.io/v7/test.png' },
edgio: { url: 'https://opt.moovweb.net/?img=/test.png' },
storyblok: { url: 'https://a.storyblok.com/test.png' },
vercel: { url: '/_vercel/image?url=/test.png&w=1536&q=100' }
},

Only use the approach I adopted for strapi if these entries really don't make sense (strapi doesn't really support most of the options that we were testing there).

If you think it needs more testing, then you can also add additional tests in the providers.test.ts file.

@sandros94
Copy link
Contributor Author

Only use the approach I adopted for strapi if these entries really don't make sense (strapi doesn't really support most of the options that we were testing there).

Directus does support all the listed ones inside providers.ts.
But before I add it there, I would like to ask if the test.png is needed or not, because Directus uses the file-id from the db to reference to the asset storage, the actual name of the file isn't needed and its mainly for download and SEO.

The location of your actual file originals is based on the project's configuration, but you can consistently access them via the API using the following URL.

example.com/assets/<file-id>
example.com/assets/1ac73658-8b62-4dea-b6da-529fbc9d01a4

SEO - You can provide an optional filename after the UUID to optimize for SEO, for example:

example.com/assets/<file-id>/<filename>
example.com/assets/1ac73658-8b62-4dea-b6da-529fbc9d01a4/directus-logo.png

This optional filename is also used in the Content-Disposition header when the ?download query parameter is used.

Any options and transformations are added as query parameters to the url.

@danielroe
Copy link
Member

test.png is just meant to be an example file name. It doesn't need to show up in the generated URL if that's not how directus works 👍

Copy link
Contributor Author

Ok, so the last commit should do it

playground/providers.ts Outdated Show resolved Hide resolved
@sandros94
Copy link
Contributor Author

I wanted to test things one last time in the playground, but I cannot make it to work anymore (last time was when I created the PR).

I do have corepack enabled and I'm following the readme, I see that a node process start to work but the nuxt server never actaully starts 🫤

It hangs here:
image

@danielroe
Copy link
Member

Would you run pnpm i to ensure you're running the latest dependencies?

@sandros94
Copy link
Contributor Author

Would you run pnpm i to ensure you're running the latest dependencies?

Pulled latest commits, pnpm i and now is working.

I started a Directus test via docker and the requests are perfectly fulfilled.
IMO we are ready to go

sandros94 and others added 2 commits June 1, 2023 18:04
Upsy.
Thanks, I was a bit on a rush.

Co-authored-by: Daniel Roe <daniel@roe.dev>
const transforms = modifiers.transforms
if (transforms && transforms.length > 0) {
// We stringify and encode in URL the list of lists, then apply it back to the modifiers
modifiers.transforms = new URLSearchParams(JSON.stringify(transforms)).toString().replace(/=+$/, '') as unknown as (string | number)[][]
Copy link
Member

Choose a reason for hiding this comment

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

This is a little bit inconsistent with other providers using ufo utils but should be fine...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the note. I'm not yet familiar with ufo, but since it is on my learning list I'll try to update this once I'm more confident

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.

Haven't locally tried but looks 💯

@pi0 pi0 merged commit 25c1426 into nuxt:main Jun 6, 2023
@pi0 pi0 mentioned this pull request Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants