Skip to content

Add a basic eslint linter config, and fixes some bugs that it caught#12

Merged
tbo47 merged 4 commits intotbo47:mainfrom
aloisklink:fix-esm-imports
Dec 14, 2022
Merged

Add a basic eslint linter config, and fixes some bugs that it caught#12
tbo47 merged 4 commits intotbo47:mainfrom
aloisklink:fix-esm-imports

Conversation

@aloisklink
Copy link
Copy Markdown
Collaborator

This is a pretty big PR that changes a bunch of things, but I tried to keep each commit separate to make things easier to review (you can view changes by commit in the GitHub review page).

If you want, I'd be happy to break this up into smaller PRs.

This PR uses eslint and eslint-plugin-import to automatically check for issues in the code.

In doing so, I caught three different types of bugs:

  • setCreateEdgeLabels was overwriting itself
  • src/dagre/debug.js was importing a file that didn't exist (wrong folder)
  • ESM imports were not ending with .js or /index.js.
    Some bundlers/typescript allow not having file extensions, but Node.JS requires them (and index.js) for files see https://nodejs.org/api/esm.html#mandatory-file-extensions

This eslint configuration was created with `npm init @eslint/config`.

I've set it up to only use ES6 compatible JavaScript, we can increase
this later if you want to support new JavaScript features.
This function was actually overwriting itself, not createEdgeLabels.
This was caught by eslint!

Fixes: 60d4b25
The [eslint-plugin-import](https://www.npmjs.com/package/eslint-plugin-import)
checks for basic ESM import issues.
In ESM, file extensions are mandatory.

Some bundlers/typescript allow not having file extensions, but
Node.JS requires them (and `index.js`) for files
see https://nodejs.org/api/esm.html#mandatory-file-extensions

I've also fixed a bug in `src/dagre/debug.js`, which was importing
a library that didn't exist (wrong folder).
Comment thread .eslintrc.cjs
module.exports = {
env: {
browser: true,
es6: true, // todo, increase this es2016 or later to use new Javascript features
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I suggest we use es2021. What do you use in marmaid-js?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

#13

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.

2 participants