adonisjs / attachment-lite

Turn any field on your Lucid models to an attachment data type
MIT License
68 stars 20 forks source link

Pre compute on demand not work #19

Closed ruanitto closed 1 year ago

ruanitto commented 2 years ago

Following the README: [...] We recommend not enabling the preComputeUrl option when you need the URL for just one or two queries and not within the rest of your application. [...]

But, in file Attachment/index.ts on line 233, the computeUrl() mehtod return nothing if the options.preComputeUrl is undefined or false

https://github.com/adonisjs/attachment-lite/blob/8144c36bf4f74e762808ba1caf631d178fd38a0e/src/Attachment/index.ts#L233-L235

Package version

1.0.7

Node.js and npm version

node: v14.18.2 npm: 6.14.15

Sample Code (to reproduce the issue)

Same code of README

ruanitto commented 2 years ago

Looking for test file, you use setOptions method:

https://github.com/adonisjs/attachment-lite/blob/8144c36bf4f74e762808ba1caf631d178fd38a0e/test/attachment.spec.ts#L96

If i set this on preCompute on demand methos, it works:

class User extends BaseModel {
  public static async preComputeUrls(models: User | User[]) {
    if (Array.isArray(models)) {
      await Promise.all(models.map((model) => this.preComputeUrls(model)))
      return
    }

    models.avatar?.setOptions({ disk: 'diskName', preComputeUrl: true }) // Only work if i replicate options of model, including preComputeUrl
    await models.avatar?.computeUrl()
    await models.coverImage?.computeUrl()
  }
}
stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

evoactivity commented 1 year ago

This should not be closed, this was very confusing.

Based on this section of the docs https://github.com/adonisjs/attachment-lite#pre-compute-on-demand

https://github.com/adonisjs/attachment-lite/blob/8144c36bf4f74e762808ba1caf631d178fd38a0e/src/Attachment/index.ts#L233

Having to replicate the options of the decorator with preComputeUrls set to true is not ideal.

I would consider adding another function meant for computing on demand where a forced flag or something can be passed to computeUrl so it ignores the value of preComputerUrls.

I can provide a patch if that sounds like an acceptable approach.

thetutlage commented 1 year ago

So far I am not sure what exactly this issue is about. Is it that preComputeUrl option not functioning as documented or something else?

evoactivity commented 1 year ago

So following the docs it's recommened to not set preComputeUrls to true on the decorator.

class User extends BaseModel {
  @attachment({ preComputeUrl: false})
  public avatar: AttachmentContract
}
class User extends BaseModel {
  public static async preComputeUrls(models: User | User[]) {
    if (Array.isArray(models)) {
      await Promise.all(models.map((model) => this.preComputeUrls(model)))
      return
    }

    await models.avatar?.computeUrl()
    await models.coverImage?.computeUrl()
  }
}

This function does not work as described. computeUrl always returns early if preComputeUrl is false or undefined. No URL's are computed.

I do not think this is a documentation issue, I would expect computeUrl to always build the urls and having preComputeUrls on the decorator to false/undefined shouldn't affect running the function on demand.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

evoactivity commented 1 year ago

not stale