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

Upgrade unist-util-visit to 4.1.0 #19

Merged
merged 7 commits into from
Jan 8, 2022

Conversation

candrews
Copy link
Contributor

@candrews candrews commented Dec 2, 2021

  • Change to using ES6 modules from CommonJS (required to use newer unist-util-visit)
  • Upgrade unist-util-visit to 4.1.0

Closes: #18

@candrews candrews changed the title Es6 unist util visit 4.1.0 Upgrade unist-util-visit to 4.1.0 Dec 2, 2021
@candrews
Copy link
Contributor Author

@sjwall could you please take a look at this PR?

@sjwall
Copy link
Owner

sjwall commented Dec 13, 2021

Hi, yes I will hopefully get chance to look at it tomorrow. Appologies for the delay

@candrews candrews force-pushed the es6-unist-util-visit-4.1.0 branch from 7c035fa to 135b622 Compare December 13, 2021 21:47
@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2021

Codecov Report

Merging #19 (ad07917) into main (51c5cb6) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #19   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           41        37    -4     
  Branches         7         7           
=========================================
- Hits            41        37    -4     
Impacted Files Coverage Δ
src/mdxast-mermaid.ts 100.00% <100.00%> (ø)
src/Mermaid.tsx 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51c5cb6...ad07917. Read the comment docs.

@@ -83,7 +83,7 @@ const config = {
/** @type {import('@docusaurus/preset-classic').Options} */
({
docs: {
remarkPlugins: [require('mdx-mermaid')],
remarkPlugins: [import('mdx-mermaid')],
Copy link
Owner

Choose a reason for hiding this comment

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

Docusaurus does not support ESM yet facebook/docusaurus#5379
I am happy to merge this change in (and make the appropriate changes to document this) if there are people wanting to use this library outside of Docusaurus.
I don't see a reason to merge this change until Docusaurus is up upgraded to support ESM if there are no use cases outside of it. The main reason for this is that it could cause confusion in the documentation and will add more support overhead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure that doesn't work?
The GitHub Action at https://github.com/candrews/mdx-mermaid/blob/135b622f14972e347b1e6b84f9303d3ea23cee43/.github/workflows/build.yml#L28 passes, meaning that the "doc" project builds (which includes the require -> import change).

Copy link
Owner

Choose a reason for hiding this comment

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

The code compiles and the React component works but the mdx parser in Docusuarus doesn't process the markdown files.
Running up the docs locally with yarn start at the bottom of intro.md there should be a diagram http://localhost:3000/mdx-mermaid/docs/intro/

Actual:
image

Expected:
image

The diagram on the index works because it is using the React component directly and not the plugin

<Mermaid chart={`
graph LR;
User-->mf[Markdown file];
mf-->cm[\`\`\`mermaid \`\`\`];
cm-->mdx[mdx-mermaid];
mdx-->Mermaid;
Mermaid-->SVG;
`}
// This isn't processed by the parser so needs config passing if it's to be configured
config={{}}/>

The import statement is returning a promise which I guess that the parser is ignoring as it doesn't know how to handle it

Choose a reason for hiding this comment

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

A dynamic import is always a promise this is why it needs to be awaited. Docusaurus itself is also using .mjs inside their own config.

Which means the docusaurus config needs to be following:

const createConfig = async () => ({
  ...
  docs: {
    remarkPlugins: [
      (await import('mdx-mermaid')).default,
    ],
  },
  ...
});

module.exports = createConfig;

With that config it should actually work.

tsconfig.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
It's best to use a specific version to ensure a consistent, reproducible
product.
@sjwall sjwall changed the base branch from main to esm January 8, 2022 16:57
@sjwall sjwall merged commit ec6fc3c into sjwall:esm Jan 8, 2022
@sjwall
Copy link
Owner

sjwall commented Jan 8, 2022

Thank you for the pull request! I have merged it into the esm branch so that I can do some more documentation work before releasing it as a version 2 ready for Docusaurus to upgrade to esm

sjwall added a commit that referenced this pull request Aug 10, 2022
* Upgrade unist-util-visit to 4.1.0 (#19)

* Set up spys instead of using mock

jest.mock doesn't work work against ES6 modules, but this approach of
using spys instead does.

See: jestjs/jest#10025

* Export as an ES6 module instead of CommonJS

* Upgrade unist-util-visit to 3.0.0

* Upgrade unist-util-visit to 4.1.0

Closes: #18

* Use `import` instead of `require`

* Use ES2020 instead of ESNext

It's best to use a specific version to ensure a consistent, reproducible
product.

Co-authored-by: Sam Wall <oss@samuelwall.co.uk>

* refactor(imports): use import type

* build(deps): bump

* fix(docs): module import

* fix(Mermaid): import theme config module

* docs: update docs for v2

* fix(reactsample): add missing type

Co-authored-by: Craig Andrews <candrews@integralblue.com>
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.

ERR_REQUIRE_ESM failure when using unist-util-visit>=3.0.0
4 participants