remove some lodash #34
Conversation
tbo47
force-pushed
the
remove-some-lodash
branch
from
dadef10
to
591f3fe
Compare
tbo47
force-pushed
the
remove-some-lodash
branch
from
958b4b2
to
9373375
Compare
|
I didn't see the comments you made on https://github.com/tbo47/dagre-es/pull/35/files I just added it to this PR. I don't want to add the types to this PR. I need to test it more. |
| @@ -46,10 +46,10 @@ var createNodes = function (selection, g, shapes) { | |||
| labelGroup.attr('id', node.labelId); | |||
| } | |||
|
|
|||
| if (_.has(node, 'width')) { | |||
| if (Object.prototype.hasOwnProperty.call(node, 'width')) { | |||
There was a problem hiding this comment.
Object.hasOwn is the replacement for this.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwn
There was a problem hiding this comment.
Thanks for your feedback! @aloisklink was saying in a earlier thread in this page that he is not for it.
There was a problem hiding this comment.
I can go both directions. I'm more for Object.hasOwn to be honest.
There was a problem hiding this comment.
Object.hasOwn was only recently added to JavaScript (after the 7.0.0 release of dagre-d3-es).
My gut feeling is that if we do want to add it, we'd probably have to make a major version bump and release dagre-d3-es v8.0.0, just in case anybody is still using Node.JS v16 or an older browser.
Maybe I'm just being overly paranoid though 🤷, but it's not too much extra effort to just find-and-replace all the Object.hasOwn calls with Object.prototype.hasOwnProperty.call.
There was a problem hiding this comment.
Speaking of which, it might be worth defining what JavaScript version this library supports.
The tsconfig.json says ES2020,
Line 6 in 805396c
If this library is aiming to be compatible with D3 v7, that still supports Node.JS v12: https://github.com/d3/d3/blob/4c01ba47177e3eb33066f87c835b9e161af1af54/package.json#L90 (although, that did come out in 2019, so it's not too long before ES2020)
There was a problem hiding this comment.
tsconfig.json and typescript are not used in the project yet.
I added it to be able to generate *.d.ts. But we are not using it yet.
The project is still a pure javascript project.
In the future, we could generate types from the source code by adding jsdoc for example. But we will probably want to be compatible with what there is in:
https://www.npmjs.com/package/@types/dagre
https://www.npmjs.com/package/@types/dagre-d3
https://www.npmjs.com/package/@types/graphlib
There was a problem hiding this comment.
I added it to be able to generate *.d.ts. But we are not using it yet.
We are publishing the auto-generated types from the .js files (although very very very basic ones).
I had a look at https://arethetypeswrong.github.io/, and every single version of dagre-d3-es since version 5.0.1 seems to have types.
If we remove them, that would cause side-effects for people.
I've made a PR to re-add *.d.ts files in #38! Thanks @tbo47 for reviewing it!
I've also checked and the ES2020 does throw an error if we try to use an JavaScript features that were added more recently then ES2020!
|
The only code which has really changed this last release is the content of this PR. @aloisklink do you want to test the code before I release a 7.0.11? |
| @@ -73,7 +71,7 @@ function findType1Conflicts(g, layering) { | |||
| return layer; | |||
| } | |||
|
|
|||
| _.reduce(layering, visitLayer); | |||
| layering.reduce(visitLayer); | |||
There was a problem hiding this comment.
I've tried testing this with the https://github.com/mermaid-js/mermaid project, and it looks like I have found an issue.
It looks like _.reduce([], myFunc) returns undefined.
However, [].reduce(myFunc) instead throws an error:
FAIL packages/mermaid/src/mermaidAPI.spec.ts > mermaidAPI > render > accessibility > stateDiagram-v2 > should set aria-roledescription to the diagram type AND should call addSVGa11yTitleDescription
TypeError: Reduce of empty array with no initial value
❯ findType1Conflicts node_modules/.pnpm/dagre-d3-es@file+..+dagre-es+dagre-d3-es-7.0.11.tgz/node_modules/dagre-d3-es/src/dagre/position/bk.js:74:12
❯ positionX node_modules/.pnpm/dagre-d3-es@file+..+dagre-es+dagre-d3-es-7.0.11.tgz/node_modules/dagre-d3-es/src/dagre/position/bk.js:354:33
❯ position node_modules/.pnpm/dagre-d3-es@file+..+dagre-es+dagre-d3-es-7.0.11.tgz/node_modules/dagre-d3-es/src/dagre/position/index.js:10:18
❯ node_modules/.pnpm/dagre-d3-es@file+..+dagre-es+dagre-d3-es-7.0.11.tgz/node_modules/dagre-d3-es/src/dagre/layout.js:42:30
❯ notime node_modules/.pnpm/dagre-d3-es@file+..+dagre-es+dagre-d3-es-7.0.11.tgz/node_modules/dagre-d3-es/src/dagre/util.js:250:10
❯ runLayout node_modules/.pnpm/dagre-d3-es@file+..+dagre-es+dagre-d3-es-7.0.11.tgz/node_modules/dagre-d3-es/src/dagre/layout.js:42:3
❯ node_modules/.pnpm/dagre-d3-es@file+..+dagre-es+dagre-d3-es-7.0.11.tgz/node_modules/dagre-d3-es/src/dagre/layout.js:19:31
❯ notime node_modules/.pnpm/dagre-d3-es@file+..+dagre-es+dagre-d3-es-7.0.11.tgz/node_modules/dagre-d3-es/src/dagre/util.js:250:10
❯ node_modules/.pnpm/dagre-d3-es@file+..+dagre-es+dagre-d3-es-7.0.11.tgz/node_modules/dagre-d3-es/src/dagre/layout.js:19:5
I think we could replace it with the following and it should be fine, though:
| layering.reduce(visitLayer); | |
| layering.length > 0 && layering.reduce(visitLayer); |
I removed the typescript migration which was not ready and transfer all other changes made since last release.