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

#46 Convert to DLL compatible plugin #83

Merged
merged 3 commits into from
Mar 14, 2023

Conversation

DanielKulbe
Copy link
Contributor

  • DLL compatible plugin build process
  • Fix some deprecations
  • Update docs

closes #46

@tony
Copy link
Collaborator

tony commented Feb 7, 2023

Wow! This is impressive!

cc'ing @isaul32 as well

@isaul32
Copy link
Owner

isaul32 commented Feb 7, 2023

This is a awesome feature. Could you @tony do the merge and release? Thanks very much.

@tony
Copy link
Collaborator

tony commented Feb 7, 2023

@isaul32 Sounds good. I will look at it this weekend (or next weekend if this one is busy)

@tony
Copy link
Collaborator

tony commented Feb 14, 2023

@DanielKulbe This will need another rebase (smaller bugfix to master)

@tony
Copy link
Collaborator

tony commented Feb 18, 2023

@DanielKulbe Give this a rebase?

@tony
Copy link
Collaborator

tony commented Feb 18, 2023

@DanielKulbe This is very good. wow!

Some issues we need to look at, consider the following diff:

Assuming yarn start:

index.html

http://localhost:8080/

diff --git a/sample/index.html b/sample/index.html
index 8ef5e67..1b26c5a 100644
--- a/sample/index.html
+++ b/sample/index.html
@@ -30,6 +30,19 @@
 <h1>CKEditor 5 with ckeditor5-math – Development Sample</h1>

 <div id="editor">
+ <h2>The <code>Math</code> plugin <h2>
+    <p>
+        <code>Math</code> inserts mathematical formulas into the editor. You can click the CKEditor 5 Math icon in the toolbar and see the results.
+    </p>
+    <p><script type="math/tex">e=mc^2</script></p>
+    <p><script type="math/tex; mode=display">e=mc^2</script></p>
+    <p>
+       This should show "\test" as ≠ via katexRenderOptions.macros:
+       <script type="math/tex">\test</script>
+    </p>
+    <!-- Quill Style Tag -->
+    <p><span class="ql-formula" data-value="e=mc^2"></span></p>
+
     <h2>Development environment</h2>
     <p>
         This is a demo of the <a href="https://ckeditor.com/docs/ckeditor5/latest/builds/guides/overview.html#classic-editor">classic editor build</a> that loads the <code>Math</code> and <code>AutoformatMath</code> plugin.

The formulas don't display in the editor itself:

image

Preview works!

image

dll.html

No editor showing for DLL builds

http://localhost:8080/dll.html

image

15Refused to execute script from '<URL>' because its MIME type ('text/html') is not executable, and strict MIME type checking is enabled.
dll.html:70 Uncaught ReferenceError: CKEditor5 is not defined
    at dll.html:70:56

@DanielKulbe
Copy link
Contributor Author

Sorry, just had have time today to rebase.

Also addressed the issues above, But for the last one, I'm not sure what's happening on your end, because it just renders fine on my end:

image

@tony
Copy link
Collaborator

tony commented Mar 2, 2023

@DanielKulbe Thank you for the rebase and checking!

Sorry about this: I still can't get http://localhost:8080/dll.html to work

I wiped node_modules/ and did yarn && yarn + yarn start

Uncaught Error: No element provided to render
    at s (auto-render.min.js:1:2575)
    at HTMLScriptElement.onload (dll.html:23:6)
15Refused to execute script from '<URL>' because its MIME type ('text/html') is not executable, and strict MIME type checking is enabled.
dll.html:68 Uncaught ReferenceError: CKEditor5 is not def

This is on:

node --version
v19.6.0

Is anyone else watching this getting http://localhost:8080/dll.html to work?

The main build at http://localhost:8080/ works fine.

@Wigny
Copy link

Wigny commented Mar 2, 2023

I don't know if that is the expected command to be run, but for me, just running yarn dll:build and later yarn dll:serve serves the DLL sample at http://127.0.0.1:8080/sample/dll.html as wanted...

Comment on lines +18 to +19
<script
src="https://cdn.jsdelivr.net/npm/[email protected]/dist/contrib/auto-render.min.js"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the Uncaught Error: No element provided to render error that happens with Katex, I think it can be solved just by adding the defer attr to the script as described in the doc.

Suggested change
<script
src="https://cdn.jsdelivr.net/npm/[email protected]/dist/contrib/auto-render.min.js"
<script
defer
src="https://cdn.jsdelivr.net/npm/[email protected]/dist/contrib/auto-render.min.js"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to remove this to prevent load errors first reported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, hey ... feel free to try and report anything that works - I'm not really familiar with Katex, as I am using MathJax in my project.

@Wigny
Copy link

Wigny commented Mar 6, 2023

@isaul32, what is missing for we have this PR merged? Looking forward to using it on my projects...

@ghalestrilo
Copy link

I have to second what @Wigny said. Our CKEditor integration at work will be so much simpler once this is merged 🥲
@tony can I kindly ask you to check this MR again? Thank you for your time.

@tony
Copy link
Collaborator

tony commented Mar 6, 2023

@ghalestrilo @Wigny @DanielKulbe I will try again tonight

@tony
Copy link
Collaborator

tony commented Mar 6, 2023

@ghalestrilo @Wigny Can you make a video of what steps you're taking to install + build and show what http://localhost:8080/dll.html looks like?

Screenshot w/ inspector

image

This still doesn't work for me 🤷.

@Wigny
Copy link

Wigny commented Mar 7, 2023

@tony, here are my steps.

I think our different outputs on it are about the commands being run to serve the DLL sample page (as I think you are just running yarn serve). I don't think ckeditor5-package-tools start does the work, so I'm testing with yarn dll:serve instead... @DanielKulbe, can you please confirm which commands should be run to test the DLL build? Moreover, I think would be nice having these steps documented in the README for help in future contributions.

Desktop.2023.03.07.-.11.13.55.04.mp4

@DanielKulbe
Copy link
Contributor Author

Yes, as you statet it some comments above:

I don't know if that is the expected command to be run, but for me, just running yarn dll:build and later yarn dll:serve serves the DLL sample at http://127.0.0.1:8080/sample/dll.html as wanted...

The whole process is already documented with the CKEditor package generator, I believe:
https://ckeditor.com/docs/ckeditor5/latest/framework/plugins/package-generator/javascript-package.html

@tony
Copy link
Collaborator

tony commented Mar 13, 2023

yarn dll:serve

@Wigny Thank you, the approach here does work for me.

@tony tony self-requested a review March 14, 2023 00:29
Copy link
Collaborator

@tony tony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielKulbe Approved

  • Tested locally with yarn dll:serve and yarn start
  • Tested on a separate build

Well done, this is awesome

@tony tony merged commit 4d5b935 into isaul32:master Mar 14, 2023
tony added a commit to tony/ckeditor5-math that referenced this pull request Mar 14, 2023
@tony tony mentioned this pull request Mar 14, 2023
tony added a commit that referenced this pull request Mar 14, 2023
@@ -1,5 +1,5 @@
{
"name": "ckeditor5-math",
"name": "@isaul32/ckeditor5-math",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DanielKulbe I did find a minor issue here.

The npm package is ckeditor5-math

When corrected, it raises:

$ yarn run dll:build
yarn run v1.22.19
$ ckeditor5-package-tools dll:build
ckeditor5-math/node_modules/@ckeditor/ckeditor5-package-tools/lib/utils/get-webpack-config-dll.js:23
        const dllName = packageJson.name.split( '/' )[ 1 ].replace( /^ckeditor5-/, '' );

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% an oversight by me 😆 Who would have thought an ordinary NPM package name would raise an error

@tony
Copy link
Collaborator

tony commented Mar 14, 2023

@isaul32 Are you okay with giving me publish access to @isaul32/ckeditor5-math on npm until ckeditor/ckeditor5-package-generator#139 is handled?

This was a mistake on my part - it flew right past me in the review, I didn't expect it'd error with our normal package name.

@isaul32
Copy link
Owner

isaul32 commented Mar 14, 2023

I'm okay with that. Do I have to do something?

@tony
Copy link
Collaborator

tony commented Mar 14, 2023

@isaul32 Yes, if you git pull master at 406aa5c, then run yarn, and yarn publish --access public - that should be enough to publish the NPM package to @isaul32/ckeditor5-math.

If that works - can you add me as a maintainer to the NPM package?

@isaul32
Copy link
Owner

isaul32 commented Mar 14, 2023

I can do it this evening. I think you are already a maintainer of the NPM package.

image

@tony
Copy link
Collaborator

tony commented Mar 14, 2023

Thank you, you will be able to see this evening

@isaul32
Copy link
Owner

isaul32 commented Mar 14, 2023

Done. I also invited you @tony again. https://www.npmjs.com/package/@isaul32/ckeditor5-math

@tony
Copy link
Collaborator

tony commented Mar 14, 2023

@isaul32 Thank you!

@tony
Copy link
Collaborator

tony commented Mar 14, 2023

@isaul32 Do you want to push the package.json with v36.0.4 to the git repo?

@DanielKulbe DanielKulbe deleted the ckeditor-dll-build branch March 23, 2023 07:07
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.

Math plugin could support CKEditor 5 DLL build
5 participants