-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Desktop: Add support for OneNote importer #10255
base: dev
Are you sure you want to change the base?
Conversation
Some failures on CI:
|
packages/app-cli/gulpfile.js
Outdated
const { execSync } = require('child_process'); | ||
|
||
const tasks = {}; | ||
|
||
const buildOneNoteConverter = () => { | ||
const profile = process.env.NODE_ENV === 'production' ? '--release' : '--debug'; | ||
return execSync(`yarn dlx wasm-pack build ../onenote-converter --target nodejs ${profile}`); | ||
}; | ||
|
||
tasks.prepareBuild = { | ||
fn: async () => { | ||
buildOneNoteConverter(); |
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.
For now, let's not support CLI. The problem is that the CLI app is installed from source on many different operating systems so we might break this for very little benefit. It's more important to support the desktop app because I expect that's what people will want to use when migrating from OneNote.
It should however be clear that OneNote import is not supported on CLI, either by not listing it in the importers or by displaying a useful error message.
|
||
const build = () => { | ||
const profile = process.env.NODE_ENV === 'production' ? '--release' : '--debug'; | ||
return execSync(`wasm-pack build --target nodejs ${profile}`); |
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.
Please don't use execSync
- we should generally only use async methods so that multiple methods can run in parallel.
You might want to use execCommand2
from the utils package instead since it will better handle errors and output
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.
Actually ignore this - in this context it's fine to reduce dependencies to other packages and use built-in Node functions instead
…lib' into add-onenote-parser-lib
This is still a draft PR.
I'm adding a new package library that will be used to convert OneNote exports into Joplin Notes.
The onenote-converter is based on the implementations found in:
https://github.com/msiemens/one2html
https://github.com/msiemens/onenote.rs
We will be compiling the Rust code to wasm with the package
wasm-pack
to output a Node package that can be imported in our codebase.Command to create node package
yarn dlx wasm-pack build ./packages/onenote-converter --target nodejs --scope joplin --debug
yarn dlx wasm-pack build ./packages/onenote-converter --target nodejs --scope joplin --release
Todos:
Right now the easiest solution to this is to ask for the user to run the CLI command to build the package before runningyarn install