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

Transform option #69

Closed
wants to merge 10 commits into from
Closed

Transform option #69

wants to merge 10 commits into from

Conversation

peterbartels
Copy link

I have added a transform option. It's a small addition with great benefit!

It can be used to transform the data from the file using official node.js transform streams, e.g. template engines (jade, coffeescript), decompression, decryption, replacing data, etc.

@dougwilson
Copy link
Contributor

The ETag and Last-Modified headers when the transform option is used match the original file; this means that browsers will cache the content based on this and if your transform is dynamic or you turn it off, users will still have the older version of the asset.

@peterbartels
Copy link
Author

Good point!

It depends on the type of transform. For example replace("a","b") will be fine but replace("a", current_time) is not.

I would suggest the default is dynamic (don't let the browser cache it) because that will work in all scenarios. It should also be possible to override it in the config in case the transform will always lead to the same result as long as the file hasn't changed. That should be mentioned in the docs.

@dougwilson
Copy link
Contributor

Sure. Feel free to update the PR for what you think the best way to solve this issue is, but the current behavior is not ideal.

@peterbartels
Copy link
Author

I have updated to code. lastModified and etag now defaults to false unless it's explicitly overridden in the options.

@TimBarham
Copy link

Hey would be really nice to get this in. @peterbartels do you plan to resolve the conflicts?

@dougwilson
Copy link
Contributor

Also, I believe this breaks when the client requests only a part of the file, because the transform stream will no longer have access to the complete file contents. For example with the replacement stream, if the file contains "foobar" and the stream is made to replace all "foo" with "bar", a normal request will get back "barbar", but a ranged request skipping the first byte would end up with "oobar" in this PR implementation.

@manifoldhiker
Copy link

manifoldhiker commented Oct 29, 2018

@dougwilson I created a failing test for this case:

Also, I believe this breaks when the client requests only a part of the file, because the transform stream will no longer have access to the complete file contents. For example with the replacement stream, if the file contains "foobar" and the stream is made to replace all "foo" with "bar", a normal request will get back "barbar", but a ranged request skipping the first byte would end up with "oobar" in this PR implementation.

      var app = http.createServer(function(req, res){
        send(req, 'test/fixtures/name.txt', {transform: function(stream) {return stream.pipe(replaceStream('to', 'pe'))}})
        .pipe(res)
      });

      request(app)
      .get('/name.txt')
      .set('Range', 'bytes=1-3')
      .expect(shouldNotHaveHeader('Last-Modified'))
      .expect(shouldNotHaveHeader('ETag'))
      .expect(206, "ebi", done)
Error: expected 'ebi' response body, got 'obi'

Can I push to this PR directly or should I reopen it somehow?

@wesleytodd
Copy link
Member

I am going to take some liberty here and close this in favor of the much more deeply discussed #195 so that we can consolidate any conversation going forward. We are not going to try and land this feature soon, as we are focusing on the v5 push for express and this is not a breaking change, but I dont want to leave too many open PRs which are unlikely to see progress, especially when they are duplicates in many ways.

@wesleytodd wesleytodd closed this May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants