Desktop,CLI: Fixes #10653: Change Resource filetype detecting strategy #10907
Conversation
packages/lib/shim-init-node.ts
Outdated
| const isUpdate = !!options.destinationResourceId; | ||
|
|
||
| const uuid = require('./uuid').default; | ||
| const { fromFile } = await import('file-type'); |
There was a problem hiding this comment.
For new code all imports should be at the top of the file. Also can we not "await" the import? Not sure what it's for, but I don't see the need for it - imports are done synchronously.
There was a problem hiding this comment.
And should be fromFile as fileTypeFromFile
There was a problem hiding this comment.
Dynamic imports returns a promise but since the import should be at the top of the file, this doesn't matter here.
There was a problem hiding this comment.
For new code all imports should be at the top of the file. Also can we not "await" the import? Not sure what it's for, but I don't see the need for it - imports are done synchronously.
I think the await import is to allow importing ESM from CommonJS. @pedr Does await import allow us to use an up-to-date version of the library?
Edit: From meeting: It doesn't.
packages/app-cli/app/command-e2ee.ts
Outdated
| @@ -8,8 +8,7 @@ import shim from '@joplin/lib/shim'; | |||
| import * as pathUtils from '@joplin/lib/path-utils'; | |||
| import { getEncryptionEnabled, localSyncInfo } from '@joplin/lib/services/synchronizer/syncInfoUtils'; | |||
| import { generateMasterKeyAndEnableEncryption, loadMasterKeysFromSettings, masterPasswordIsValid, setupAndDisableEncryption } from '@joplin/lib/services/e2ee/utils'; | |||
| const imageType = require('image-type'); | |||
| const readChunk = require('read-chunk'); | |||
| import { fromFile } from 'file-type'; | |||
|
Any reason why there's no unit tests? |
Sorry, I thought it would be harder to test but I couldn't find a way to test |
packages/lib/fixtures/test_pdf
Outdated
| @@ -0,0 +1,198 @@ | |||
| %PDF-1.3 | |||
There was a problem hiding this comment.
Support test files should go in packages/app-cli/tests/support which you can then access using the supportDir constant
|
@pedr there are some conflicts that need to be resolved |
Fixes #10653
Description
It is not a very big change, I'm exchanging the requirement of Desktop and CLI by changing the detecting filetype library to
file-typeGithub NPMThe library is from the same author from the
image-typethat we were using until this point, but it isn't needed anymore sincefile-typeis a "super set" ofimage-typedetection.Important: I chose to use an older version of
file-typesince the current version only supports ESM, which our project has not fully supported yet.https://github.com/sindresorhus/file-type/releases/tag/v16.5.4
Testing