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

Desktop: Add support for OneNote importer #10255

Draft
wants to merge 51 commits into
base: dev
Choose a base branch
from

Conversation

pedr
Copy link
Collaborator

@pedr pedr commented Apr 3, 2024

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:

  • Try to find a better way for the first install of project
    • Right now the easiest solution to this is to ask for the user to run the CLI command to build the package before running yarn install
    • We just use a package.json to point to the real files that don't exist yet
    • fix error in CI
  • Disable import in app-cli

@laurent22
Copy link
Owner

Some failures on CI:

2024-04-03T13:45:29.7543910Z ➤ YN0000: ┌ Resolution step
2024-04-03T13:45:29.7545630Z ##[group]Resolution step
2024-04-03T13:45:30.7137160Z ➤ YN0001: │ Error: onenote-converter@file:../onenote-converter/pkg::locator=%40joplin%2Flib%40workspace%3Apackages%2Flib: ENOENT: no such file or directory, lstat '/Users/runner/work/joplin/joplin/packages/onenote-converter/pkg'
2024-04-03T13:45:30.7346050Z ##[endgroup]
2024-04-03T13:45:30.7351150Z ➤ YN0001: Error: onenote-converter@file:../onenote-converter/pkg::locator=%40joplin%2Flib%40workspace%3Apackages%2Flib: ENOENT: no such file or directory, lstat '/Users/runner/work/joplin/joplin/packages/onenote-converter/pkg'
2024-04-03T13:45:30.7355690Z ➤ YN0000: └ Completed in 0s 957ms
2024-04-03T13:45:30.7357660Z ➤ YN0000: Failed with errors in 0s 966ms
2024-04-03T13:45:30.7632990Z Yarn installation failed. Search for 'exit code 1' in the log for more information.
2024-04-03T13:45:30.7652800Z ##[error]Process completed with exit code 1.
2024-04-03T13:45:30.7811060Z Post job cleanup.

Comment on lines 4 to 15
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();
Copy link
Owner

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}`);
Copy link
Owner

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

Copy link
Owner

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

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

2 participants