-
-
Notifications
You must be signed in to change notification settings - Fork 762
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
Write using saved mime type #403
Conversation
This allows filenames without a suffix to save properly using the already established MIME type. Closes jimp-dev#114. Though, this doesn't add any functionality for changing the mime type as referenced in the issue jimp-dev#114. Note: MIME.lookup(path) is actually already being used to obtain the value that this.getMIME() references.
@qw3n If you update this I'll merge it |
make the same update to line 230 in the README too https://github.com/oliver-moran/jimp/pull/429/files |
Are the updates your referencing the resolution of the conflicts and the readme or is there something else? |
Yeah. I need you to:
|
If neither is set getMIME defaults to type PNG
Patch 2
Added an example to the code as well as mentioning that it all defaults to PNG
Inferred MIME types
I'm looking at writing a test, but I don't see one for the write method. So starting from scratch I'm not sure what the best method is for testing that function specifically. For example is there a way of testing it without actually saving the image or even needing a sample image. |
Look at the async tests. the one called 'write returns promise' writes and deletes a file |
So I'm having a little trouble figuring out how to test for the MIME type of the image without using the path extension. Do you have any suggestions? |
src/index.js
Outdated
@@ -397,7 +397,7 @@ class Jimp extends EventEmitter { | |||
return throwError.call(this, 'cb must be a function', cb); | |||
} | |||
|
|||
const mime = MIME.getType(path); | |||
const mime = MIME.getType(path) || this.getMime(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't the name of the function. It's actually getMIME
it('write uses original MIME type', done => {
const writePath = './test.png';
if (process.env.BABEL_ENV === undefined) {
return done();
}
new Jimp(imagesDir + '/dice.png', function(err) {
if (err) done(err);
this.write(writePath.split('.png')[0], (err, image) => {
if (err) done(err);
should.exist(image);
fs.existsSync(writePath).should.be.true();
fs.unlinkSync(writePath);
done();
});
});
}); |
So you're saying if there is no extension the code should force add it to the path name. The original reason I ran into this was in working with file uploads. You can say where you want to save the file, but the default multer naming mechanism merely gives it a hash name with no extension. Which didn't seem to be a problem since I was storing the name in a database and I had already limited uploads to jpg and png. With this though I would have to have another step to adjust the database entry after jimp has modified the file name. Just something to consider. Also, this was supposed to be a simple edit didn't realize it would get quite so complicated. |
Can you even write to the files system without an extension? I haven’t
tested that out. I see where you’re coming from now. Can you confirm if fs
will even write without a file extension?
…On Tue, Jul 31, 2018 at 9:10 PM qw3n ***@***.***> wrote:
So you're saying if there is no extension the code should force add it to
the path name.
The original reason I ran into this was in working with file uploads. You
can say where you want to save the file, but the default multer naming
mechanism merely gives it a hash name with no extension. Which didn't seem
to be a problem since I was storing the name in a database and I had
already limited uploads to jpg and png. With this though I would have to
have another step to adjust the database entry after jimp has modified the
file name. Just something to consider.
Also, this was supposed to be a simple edit didn't realize it would get
quite so complicated.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#403 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABIyBEJj7POQkFqKDyZq4_94ZEFtm0jzks5uMSokgaJpZM4SCkI_>
.
|
Couldn’t you just get the buffer and ace as you want to? That would probably be simpler than having jimp wrote the file @qw3n |
I know you can write without extensions, because six months ago I modified my local jimp copy with the code from the original pull request and it worked fine. In my particular use case with multer I wasn't using jimp to write the original file. I was using it to create a thumbnail copy in a thumbs folder with the same name as the original. Multer comes with an option to automatically name uploaded files based on a hash, however, you don't have any control over the name. If I had absolutely had to have extensions multer provides a hook to name a file, but then I would have had to pull in a package to do the exact same hashing etc. all so I could then add an extension on the end. This is just my experience, but given the way multer worked it just seemed easier to ignore extensions since the browser uses content headers not extensions to determine filetype. Also, assuming you decide extensions aren't required, then you could use the file-type module to get the MIME type of the generated image, but the image would have to loaded and then converted to a buffer. Is that feasible or would that slow down the tests to much? |
I'm fine with saving without an extension. I just can't get the code to work Here is my current write function with your changes write(path, cb) {
if (!FS || !FS.createWriteStream) {
throw new Error(
'Cant access the filesystem. You can use the getBase64 method.'
);
}
if (typeof path !== 'string') {
return throwError.call(this, 'path must be a string', cb);
}
if (typeof cb === 'undefined') {
cb = noop;
}
if (typeof cb !== 'function') {
return throwError.call(this, 'cb must be a function', cb);
}
const mime = MIME.getType(path) || this.getMIME();
const pathObj = Path.parse(path);
if (pathObj.dir) {
MkDirP.sync(pathObj.dir);
}
this.getBuffer(mime, (err, buffer) => {
if (err) {
return throwError.call(this, err, cb);
}
const stream = FS.createWriteStream(path);
stream
.on('open', () => {
stream.write(buffer);
stream.end();
})
.on('error', err => {
return throwError.call(this, err, cb);
});
stream.on('finish', () => {
return cb.call(this, null, this);
});
});
return this;
} and the test i provided I keep getting this error:
|
The error itself was telling us the problem. Took me way to long to realize we already have a test directory so that is what fs is establishing a connection to. If you change the name to |
This allows filenames without a suffix to save properly using the already established MIME type. Closes #114. Though, this doesn't add any functionality for changing the mime type as referenced in the issue #114.
Note: MIME.lookup(path) is actually already being used to obtain the value that this.getMIME() references.