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

Add support for AssetInfo #23

Merged
merged 1 commit into from Dec 18, 2015
Merged

Add support for AssetInfo #23

merged 1 commit into from Dec 18, 2015

Conversation

keegancsmith
Copy link
Contributor

Currently only uses ModTime from AssetInfo.

Currently only uses `ModTime` from `AssetInfo`.
@keegancsmith
Copy link
Contributor Author

I think this will currently break the API of anyone relying on NewAsset* or who is using positional arguments on AssetFS. I'm not sure what the best approach to move forward is, but this is a nice change to have.

Motivation: Currently the modtime returned is the time of the binary starting. If you have two instances behind a reverse-proxy they will return different modtimes. This causes cache control headers to frequently invalidate.

@dmitshur
Copy link
Contributor

I could be wrong, but it seems unlikely that there would be code relying on NewAsset* since the whole point of this library is to expose the output of go-bindata as a http.FileSystem interface. In fact, I'm not sure why that func is exported, since it seems to be a part of internal API of go-bindata-assetfs.

It would break if using unkeyed composite literals, but not if using field-keyed syntax.

It's not up to me to make API decisions here, but it seems like a reasonable and favorable change. go-bindata already captures and exposes modtimes, and it's a part of http.FileSystem interface too. My vfsgen fork already has this feature, it preserves the modtimes of input.

The implementation details LGTM.

@keegancsmith
Copy link
Contributor Author

@elazarl ping

elazarl added a commit that referenced this pull request Dec 18, 2015
Add support for AssetInfo, custom file info function
@elazarl elazarl merged commit 8731e8b into elazarl:master Dec 18, 2015
@elazarl
Copy link
Owner

elazarl commented Dec 18, 2015

@keegancsmith thanks, apologize for the delay. Sounds like a good idea.

@alde
Copy link

alde commented Dec 22, 2015

Just FYI:

This causes a runtime error if you don't specify AssetInfo when creating the struct, and later try to access the data.

See eremetic-framework/eremetic#44

@ryanuber
Copy link

Just adding a note here for anyone who comes looking for this answer: if you start seeing panics after this change, it could be that you need to re-generate and update the output of go-bindata-assetfs. In my case, we were checking the resulting Go code into the repo after generation, and saw these panics during testing:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x7e904e]
goroutine 6499 [running]:
testing.tRunner.func1(0xc82150c5a0)
    /usr/local/go/src/testing/testing.go:450 +0x171
github.com/elazarl/go-bindata-assetfs.(*AssetFS).Open(0xc821e0bef0, 0xc82164e2b0, 0xf, 0x0, 0x0, 0x0, 0x0)
    /home/travis/gopath/src/github.com/elazarl/go-bindata-assetfs/assetfs.go:148 +0x22e
net/http.serveFile(0x2b9f5b923f80, 0xc821f980a0, 0xc8217a9880, 0x2b9f5b954b88, 0xc821e0bef0, 0xdfd190, 0x1, 0xdfd201)
    /usr/local/go/src/net/http/fs.go:394 +0x5b1
...
...

@elazarl
Copy link
Owner

elazarl commented Dec 25, 2015

Hmm... I was assuming that people who update their go-bindata-assetfs dir,
would also rerun the binary.

Do you think it's better to revert this change? I'm not sure.

On Fri, Dec 25, 2015 at 5:04 AM, Ryan Uber notifications@github.com wrote:

Just adding a note here for anyone who comes looking for this answer: if
you start seeing panics after this change, it could be that you need to
re-generate and update the output of go-bindata-assetfs. In my case, we
were checking the resulting Go code into the repo after generation, and saw
these panics during testing:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0x0 pc=0x7e904e]
goroutine 6499 [running]:
testing.tRunner.func1(0xc82150c5a0)
/usr/local/go/src/testing/testing.go:450 +0x171github.com/elazarl/go-bindata-assetfs.(*AssetFS).Open(0xc821e0bef0, 0xc82164e2b0, 0xf, 0x0, 0x0, 0x0, 0x0)
/home/travis/gopath/src/github.com/elazarl/go-bindata-assetfs/assetfs.go:148 +0x22e
net/http.serveFile(0x2b9f5b923f80, 0xc821f980a0, 0xc8217a9880, 0x2b9f5b954b88, 0xc821e0bef0, 0xdfd190, 0x1, 0xdfd201)
/usr/local/go/src/net/http/fs.go:394 +0x5b1
...
...


Reply to this email directly or view it on GitHub
#23 (comment)
.

@@ -137,7 +144,11 @@ func (fs *AssetFS) Open(name string) (http.File, error) {
name = name[1:]
}
if b, err := fs.Asset(name); err == nil {
return NewAssetFile(name, b), nil
timestamp := defaultFileTimestamp
Copy link
Contributor

Choose a reason for hiding this comment

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

You could add something like this here, to prompt people to re-generate:

if fs.AssetInfo == nil {
    log.Fatalln("go-bindata-assetfs: you're using a new version of the library but haven't re-generated the file, so it's not compatible; regenerate to avoid this problem")
}

Or something like that.

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.

None yet

5 participants