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

Invalid synatax for mermaid.core.mjs #4094

Closed
Mister-Hope opened this issue Feb 16, 2023 · 15 comments · Fixed by #4108
Closed

Invalid synatax for mermaid.core.mjs #4094

Mister-Hope opened this issue Feb 16, 2023 · 15 comments · Fixed by #4108
Labels
Status: Triage Needs to be verified, categorized, etc Type: Bug / Error Something isn't working or is incorrect

Comments

@Mister-Hope
Copy link
Contributor

Mister-Hope commented Feb 16, 2023

Description

When using extension mjs, we are working under es module, which file extension is required.

However, the following code appears in dist filei:

function loadLocale(name2) {
  var oldLocale = null, aliasedRequire;
  if (locales[name2] === void 0 && typeof module !== "undefined" && module && module.exports && isLocaleNameSane(name2)) {
    try {
      oldLocale = globalLocale._abbr;
      aliasedRequire = require;
      aliasedRequire("./locale/" + name2);
      getSetGlobalLocale(oldLocale);
    } catch (e) {
      locales[name2] = null;
    }
  }
  return locales[name2];
}

So I am getting an error log with webpack:

demo/md-enhance demo:webpack-build:   moduleIdentifier: '/home/runner/work/vuepress-theme-hope/vuepress-theme-hope/node_modules/.pnpm/[email protected]/node_modules/mermaid/dist/mermaid.5fa68f79.js',
demo/md-enhance demo:webpack-build:   moduleName: '../../node_modules/.pnpm/[email protected]/node_modules/mermaid/dist/mermaid.5fa68f79.js',
demo/md-enhance demo:webpack-build:   loc: '1487:6-41',
demo/md-enhance demo:webpack-build:   message: "Module not found: Error: Can't resolve './locale' in '/home/runner/work/vuepress-theme-hope/vuepress-theme-hope/node_modules/.pnpm/[email protected]/node_modules/mermaid/dist'",
demo/md-enhance demo:webpack-build:   moduleId: 8107,
demo/md-enhance demo:webpack-build:   moduleTrace: [
demo/md-enhance demo:webpack-build:     {
demo/md-enhance demo:webpack-build:       originIdentifier: 'javascript/esm|/home/runner/work/vuepress-theme-hope/vuepress-theme-hope/node_modules/.pnpm/[email protected]/node_modules/mermaid/dist/mermaid.esm.min.mjs',
demo/md-enhance demo:webpack-build:       originName: '../../node_modules/.pnpm/[email protected]/node_modules/mermaid/dist/mermaid.esm.min.mjs',
demo/md-enhance demo:webpack-build:       moduleIdentifier: '/home/runner/work/vuepress-theme-hope/vuepress-theme-hope/node_modules/.pnpm/[email protected]/node_modules/mermaid/dist/mermaid.5fa68f79.js',
demo/md-enhance demo:webpack-build:       moduleName: '../../node_modules/.pnpm/[email protected]/node_modules/mermaid/dist/mermaid.5fa68f79.js',
demo/md-enhance demo:webpack-build:       dependencies: [Array],
demo/md-enhance demo:webpack-build:       originId: 783,
demo/md-enhance demo:webpack-build:       moduleId: 8107
demo/md-enhance demo:webpack-build:     },

Steps to reproduce

...

Screenshots

No response

Code Sample

No response

Setup

No response

Additional Context

No response

@Mister-Hope Mister-Hope added Status: Triage Needs to be verified, categorized, etc Type: Bug / Error Something isn't working or is incorrect labels Feb 16, 2023
@sidharthv96
Copy link
Member

Might be related to #4034

@sidharthv96
Copy link
Member

sidharthv96 commented Feb 16, 2023

image

image

I'm getting this when trying to update live editor.

@emersonbottero @aloisklink should we revert #4034 for now and release 9.4.1?
Could be an issue with bundler, will have to check.

@AielloChan
Copy link
Contributor

image

Because of moment's dynamic require locate files, the require statement still use relative path, even in mermaid, thus can't find './locate' files.

Mermaid shouldn't pack "moment" in its dist file! Just keep require node_modules module.

@Mister-Hope
Copy link
Contributor Author

Mister-Hope commented Feb 18, 2023

Because of moment's dynamic require locate files, the require statement still use relative path, even in mermaid, thus can't find './locate' files.\nMermaid shouldn't pack "moment" in its dist file! Just keep require node_modules module.

I don't think so, it should try to pack locales it choose and drop that part of code. Maybe with an edited moment package while packing.

With cdn and scripts, every thing is expected to be packed.

Btw, I am curious why don't we choose dayjs

@sidharthv96
Copy link
Member

Moment is an old dependency.
I remember someone had done a POC to replace it with a new lib, but that didn't pan out.

Moment isn't a major dependency and we're open to replace it as long as the functionality isn't affected.

@sidharthv96
Copy link
Member

I have reverted to moment-mini for now.
@Mister-Hope can you test [email protected]?

@aloisklink
Copy link
Member

I have reverted to moment-mini for now.

👍 Agree, that we should replace moment when we can.

Btw, I am curious why don't we choose dayjs

I've actually got a proof-of-concept commit here: aloisklink@3964cc5.

The current major issue seems to be that date.add(dayjs.duration(1, "day")) always adds 24 hours instead of 1 day (different from Moment.js) (see iamkun/dayjs#1271)

This causes issues with daylight savings, as those days have 23/25 hours.

Most annoyingly, it seems to depend on daylight savings on your PC, so dayjs("2023-11-05").add(dayjs.duration(1, "day")) will work fine in Arizona, USA, but if you bring your laptop to California, USA and it automatically updates the timezones, suddenly your code will stop working.

What we could do is maybe use dayjs for the formatting/date-parsing, and then use Temporal.Duration for adding days to dates (technically Temporal is experimental only, but we can just use a polyfill until it's in all the major browsers).

@sidharthv96
Copy link
Member

@aloisklink, was that the only issue?

Did all the unit tests pass?

Many of them failed for me with dates being different ~15days.

Also, please add a unit test for the issue you identified if it's not already covered.

@Mister-Hope
Copy link
Contributor Author

I have reverted to moment-mini for now.
@Mister-Hope can you test [email protected]?

It worked in some cases, at least the final code is correct, bu tai still met js heap out of space so there are still memory leak in it.

@sidharthv96
Copy link
Member

sidharthv96 commented Feb 20, 2023

Can you share a reproduction for the memory issues?
I only got in on build time, not runtime.

During runtime, the memory footprint was actually smaller for 9.4 than 9.3

@sidharthv96 sidharthv96 linked a pull request Feb 20, 2023 that will close this issue
10 tasks
@aloisklink
Copy link
Member

@aloisklink, was that the only issue?

Did all the unit tests pass?

Yep, that was the only issue. One unit test failed, but that was because I'm in a region that currently follows daylight savings, and a unit test went across a daylight savings boundary.

One difference with dayjs from moment is that dayjs objects are immutable.

E.g.

const a = moment("2020-01-01");
a.add(1, "day");
console.log(a); // 2020-01-02

const b = dayjs("2020-01-01");
const c = b.add(1, "day"); // does not change b, since dayjs objects are immutable
console.log(b); // 2020-01-01
console.log(c); // 2020-01-02

Also, please add a unit test for the issue you identified if it's not already covered.

I had a quick try, but unfortunately, this issues depends on the timezone of the PC that is running the unit tests. Setting process.env.TZ to change the timezone in vitest doesn't work (see vitest-dev/vitest#1575).

We could add a CI action that runs the tests with a custom timezone, but that seems like a lot of effort, so I'm not 100% sure if it's worth it.

@sidharthv96
Copy link
Member

Yeah, not worth the effort, then.
Can you raise the PR for dayjs?

@aloisklink
Copy link
Member

Yeah, not worth the effort, then.

But then again, you're right, we do need to somehow test for daylight savings issues (even if it's just a manual test). Maybe we need to make a new issue called "Removing moment as a dependency of mermaid" and stick daylight savings testing as a core requirement.

Can you raise the PR for dayjs?

I can have a look. I've made PRs to dayjs before (for a different daylight savings issue), and the dayjs code is really hard to work with. It's basically written pre-minified to save space, so all object properties have single character names, like a.b.c.

Considering that the issue has been open for 2 years, it might not be the easiest fix in the world.

My gut feeling is to just use a Temporal.Duration polyfill instead. The API for Temporal isn't yet supported by any browser, since the API isn't 100% locked down, but we can use the polyfill temporarily and then remove it once most browsers support it.

@sidharthv96
Copy link
Member

Can you raise the PR for dayjs?

@aloisklink , oops! I meant a PR in mermaid with your changes containing dayjs. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Triage Needs to be verified, categorized, etc Type: Bug / Error Something isn't working or is incorrect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants