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

Esmodule version of the introduction of hast-util-from-dom leads to errors... #56

Closed
rxliuli opened this issue Mar 18, 2021 · 12 comments
Closed

Comments

@rxliuli
Copy link

rxliuli commented Mar 18, 2021

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch [email protected] for the project I'm working on.

Here is the diff that solved my problem:

diff --git a/node_modules/rehype-mathjax/lib/renderer.js b/node_modules/rehype-mathjax/lib/renderer.js
index 1a6c927..a724d53 100644
--- a/node_modules/rehype-mathjax/lib/renderer.js
+++ b/node_modules/rehype-mathjax/lib/renderer.js
@@ -1,6 +1,6 @@
 const mathjax = require('mathjax-full/js/mathjax').mathjax
 const register = require('mathjax-full/js/handlers/html').RegisterHTMLHandler
-const fromDom = require('hast-util-from-dom')
+const fromDom = require('hast-util-from-dom').default
 const toText = require('hast-util-to-text')
 const createAdaptor = require('./adaptor')
 

This issue body was partially generated by patch-package.

image

repo: https://github.com/rxliuli/joplin-search-intergration/blob/ff95928d5e02823cb2738ef72d826196aea09ade/src/pages/options/pages/note/util/render.ts#L27

@tani
Copy link
Contributor

tani commented Mar 18, 2021

First, I am not a member of this project, but I am original author of this package.
I have been also faced this problem about a decade month ago.
I guess this is an issue of hast-util-from-dom, which package should be compiled as an esmodule.
The es6 and esmodule support is the ongoing task of this porject. Please read unifiedjs/rfcs#4 .

@wooorm
Copy link
Member

wooorm commented Mar 18, 2021

This is also definitely something your bundler can fix. And some of them do. So you might be able to solve it there.

@rxliuli
Copy link
Author

rxliuli commented Mar 18, 2021

This is also definitely something your bundler can fix. And some of them do. So you might be able to solve it there.

I tried it in the create-react-app failed, I believe that most applications will fail.

@wooorm
Copy link
Member

wooorm commented Mar 18, 2021

webpack 4 has a lot of bugs. But you’re out of luck. CRA prevents you from working on that.

@rxliuli
Copy link
Author

rxliuli commented Mar 18, 2021

webpack 4 has a lot of bugs. But you’re out of luck. CRA prevents you from working on that.

I don't know if WebPack has an error, but I know that if WebPack / Rollup can't be compatible, it will use it very unfriendly.

@wooorm
Copy link
Member

wooorm commented Mar 20, 2021

I don't know if WebPack has an error, but I know that if WebPack / Rollup can't be compatible, it will use it very unfriendly.

Webpack 5, Rollup, and Node support the way it is now. Your patch will break them.
For CRA, see: facebook/create-react-app#9994.


We (+ much more of the JS ecosystem) are moving to full ESM over the coming months: unifiedjs/unified#121

@rxliuli
Copy link
Author

rxliuli commented Mar 20, 2021

I don't know if WebPack has an error, but I know that if WebPack / Rollup can't be compatible, it will use it very unfriendly.

Webpack 5, Rollup, and Node support the way it is now. Your patch will break them.
For CRA, see: facebook/create-react-app#9994.

We (+ much more of the JS ecosystem) are moving to full ESM over the coming months: unifiedjs/unified#121

CRA migration WebPack 5 is a long-term process, so far, it has been completed 24%, then, the remark-math will be unavailable during this time before really complete. I am not opposing to support better ESModule standards, just want to remind, do compatibility processing. . .for example, now I can't use the CRA and ROLLUP package, so I can only use Patch-Node to modify until a certain time Remark-Math directly introduces no problem.

I use the remark-math project, use the Rollup package. https://github.com/rxliuli/joplin-search-intergration/blob/ff95928d5e/rollup.config.js

@wooorm
Copy link
Member

wooorm commented Mar 20, 2021

Why did you mention CRA before if you are now talking about (+ the links you share) Rollup?

do compatibility processing

This package is CJS. It uses a require to load a dual CJS/ESM package. Your bundler should use the CJS file. If it doesn’t, it’s either a bug there or it has to be configured in some way.

@rxliuli
Copy link
Author

rxliuli commented Mar 20, 2021

Why did you mention CRA before if you are now talking about (+ the links you share) Rollup?

do compatibility processing

This package is CJS. It uses a require to load a dual CJS/ESM package. Your bundler should use the CJS file. If it doesn’t, it’s either a bug there or it has to be configured in some way.

So how should rollup configure the hast-util-from-dom using a CommonJS version of rehype-mathjax?

@wooorm
Copy link
Member

wooorm commented Mar 20, 2021

I guess this is an issue of hast-util-from-dom, which package should be compiled as an esmodule.

Re this earlier message by @tani, what was your solution?

Why did you mention CRA before if you are now talking about (+ the links you share) Rollup?

@rxliuli Can you answer this earlier question?
Otherwise, can you make a small reproduction: your project is giant, and I don’t have time to debug everything?

@rxliuli
Copy link
Author

rxliuli commented Mar 20, 2021

I guess this is an issue of hast-util-from-dom, which package should be compiled as an esmodule.

Re this earlier message by @tani, what was your solution?

Why did you mention CRA before if you are now talking about (+ the links you share) Rollup?

@rxliuli Can you answer this earlier question?
Otherwise, can you make a small reproduction: your project is giant, and I don’t have time to debug everything?

Because I also need to use MarkDown in the CRA project in the production environment, ITMDOWN-IT is currently used, but I want to unify them.

@rxliuli
Copy link
Author

rxliuli commented Mar 20, 2021

Forget it, I temporarily close this problem. Anyway, the question has been temporarily solved, and there is at least other options if not. . .

@rxliuli rxliuli closed this as completed Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants