Skip to content

Configuration to file path for attachment#11

Open
jonstorer wants to merge 7 commits into
achingbrain:masterfrom
shake-apps:configure-file-name
Open

Configuration to file path for attachment#11
jonstorer wants to merge 7 commits into
achingbrain:masterfrom
shake-apps:configure-file-name

Conversation

@jonstorer
Copy link
Copy Markdown

DO NOT MERGE THIS

this won't pass on travis until the tungus issues are cleared up

I plan on cleaning this up a lot before it's ready to merge. But please provide as much feedback as you can. Thanks!
#10

This is working. However, I had issues with Tungus.

This change sets target on the attachment. When then attachment is passed to the storageProvider, the provider can use it or ignore it.

target is set 3 ways.

// default
// :modelName/:fieldName/:id

schema.plugin(crate, {
  storage: new Storage(),
  fields: { file: { } }
})
// options for the model
// url:

schema.plugin(crate, {
  url: ':modelName/files/:updated_at/:id'
  storage: new Storage(),
  fields: { file: { } }
})
// per attachment
// url:

schema.plugin(crate, {
  storage: new Storage(),
  fields: {
    file: {
      url: ':collectionName/:id-:updated_at'
    }
  }
})

Todo

  • update README
  • provide examples

Comment thread lib/Crate.js
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm using this here instead of model, because model can sometimes be something other than a model for array type fields.

Comment thread lib/TargetBuilder.js Outdated
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

4 - 28, seems complicated, but it makes 40-43 simple. Any refactoring suggestions?

Comment thread test/fixtures/Storage.js
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

check out this awesome storage provider using the target property!

@achingbrain
Copy link
Copy Markdown
Owner

Thanks for this - looks like you're putting a ton of work in. I'm away from the computer for a few days - I'll take an in depth look when I get back.

Comment thread lib/Crate.js
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Be careful with your casing - the new file is TargetBuilder but you are importing targetBuilder which will work on case-insensitive file systems (e.g. Windows, Mac OS X) but fail on case sensitive ones (e.g. most Linux file systems)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

My bad, missed that in refactoring. In the first pass, targetBuilder was a constructor.

@jonstorer
Copy link
Copy Markdown
Author

@achingbrain I simplified the target builder.

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.

2 participants