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: Fixes #11408: Correct file path of OneNote converter on release build #11410

Merged
merged 3 commits into from
Nov 27, 2024

Conversation

pedr
Copy link
Collaborator

@pedr pedr commented Nov 18, 2024

Fixes #11408

Summary

When we added onenote-converter to 3.2.3 I end-up forgetting to test the release build.

Testing it I found out that the requireDynamic call that we have on InteropService_Importe_OneNote was pointing it out to a file that doesn't exist on the release build.

The file doesn't exist there because the path that was being used is from the file to packages/onenote-converter, instead of pointing it out to the module inside lib lib/node_modules/@joplin/onenote-convert.

Testing

Testing on dev

  1. Run git clean -dfx in the root directory of Joplin
  2. Run yarn
  3. Run cd packages/onenote-converter && IS_CONTINUOUS_INTEGRATION=1 yarn build
  4. Run cd ../app-desktop && yarn start
  5. File -> Import -> ZIP - OneNote Importer
  6. Select any ZIP file
  7. The error reported in the issue shouldn't be present

Testing on release

  1. Run git clean -dfx in the root directory of Joplin
  2. Run yarn
  3. Run cd packages/onenote-converter && IS_CONTINUOUS_INTEGRATION=1 yarn build
  4. Run cd ../app-desktop && yarn dist
  5. Run dist/Joplin-3.2.3.AppImage
  6. File -> Import -> ZIP - OneNote Importer
  7. Select any ZIP file
  8. The error reported in the issue shouldn't be present

@pedr pedr added bug It's a bug desktop All desktop platforms labels Nov 18, 2024
@pedr pedr requested a review from laurent22 November 18, 2024 19:22
@pedr pedr self-assigned this Nov 18, 2024
@laurent22
Copy link
Owner

Normally we put such files in the build directory so that there's a stable path to get to it (See ElectronAppWrapper::buildDir). Because now if I'm not mistaken it's going to work on release but no longer on dev mode, is that correct?

@pedr
Copy link
Collaborator Author

pedr commented Nov 21, 2024

  • There isn't way a to remove the dynamic import? Maybe a file only for mobile (eg: file.android.js)

@pedr
Copy link
Collaborator Author

pedr commented Nov 22, 2024

I was looking why I chose to use shim.requireDynamic because I couldn't remember, what happened is that onenote-converter is a dependency of lib which is required by the server.

To avoid adding Rust to the docker server image I end up removing the lines that imports onenote-converter and using requireDynamic. See commit: 992820c#diff-bdfb8b5c7b1ac62ecd6d5857c4f634d1b5c716ca76774f306c341f17ae7644e6

I'm trying to think if there is anything else I can do to avoid this.

Error that happens if I try to build without requireDynamic:

#0 198.3 ➤ YN0000: │ root@workspace:. STDERR ➤ YN0000: [@joplin/lib]: Process exited (exit code 2), completed in 22s 130ms
#0 198.3 ➤ YN0000: │ root@workspace:. STDERR ➤ YN0000: Done in 22s 145ms
#0 198.3 ➤ YN0000: │ root@workspace:. STDERR     at makeError (/build/node_modules/execa/lib/error.js:60:11)
#0 198.3 ➤ YN0000: │ root@workspace:. STDERR     at handlePromise (/build/node_modules/execa/index.js:118:26)
#0 198.3 ➤ YN0000: │ root@workspace:. STDERR     at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
#0 198.3 ➤ YN0000: │ root@workspace:. STDERR     at async execCommand (/build/gulpfile.js:27:17)
#0 198.3 ➤ YN0000: │ root@workspace:. STDERR     at async fn (/build/gulpfile.js:62:5)
#0 198.3 ➤ YN0009: │ root@workspace:. couldn't be built successfully (exit code 1, logs can be found here: /tmp/xfs-776bde40/build.log)
#0 198.3 ➤ YN0000: └ Completed in 1m 52s
#0 198.7 ➤ YN0000: Failed with errors in 3m 17s

(It can't find package because it was deleted with sed in the Dockerfile.server definition):

# We don't want to build onenote-converter since it is not used by the server
RUN sed --in-place '/onenote-converter/d' ./packages/lib/package.json

@pedr
Copy link
Collaborator Author

pedr commented Nov 25, 2024

  • I don't need to call the specific file (@joplin/onenote should be enough)

@pedr
Copy link
Collaborator Author

pedr commented Nov 26, 2024

I updated the PR as requested, testing again on dev and release version on Linux.

I also removed some information from the package.json that wasn't being used (requireDynamic can't return the typescript type, so I removed it from this entry also)

@laurent22
Copy link
Owner

That looks good now, thanks Pedro

@laurent22 laurent22 merged commit 2f3b388 into laurent22:dev Nov 27, 2024
7 checks passed
@pedr pedr deleted the onenote-converter-not-working branch November 27, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug It's a bug desktop All desktop platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

One Note importer is not working on release
2 participants