Continue the development based on source code from tylingsoft/dagre-layout

[tylerlong]

Disclaimer

This is just a proposal. I think it is good for the future of dagre but it's just my opinion.

About tylingsoft/dagre-layout

I am the author of tylingsoft/dagre-layout. It is essentially a forked version of dagre. It was started in September 2017, back then dagre was not being actively developed and I decided to make lots of major changes. So started it as new repo instead of a forked repo.

Changes I have made

1. Upgrade all the dependencies (including loadash 3 => 4)
2. Yarn instead of NPM
3. Get rid of PhantomJS
4. ES6 instead of ES5
5. Use webpack instead of browserify
6. Use JavaScript Standard Style instead of JSHint
7. Use Jest for testing
8. Add test coverage report
9. Remove Bower support
10. Use 0 instead of Number.NEGATIVE_INFINITY
11. Git ignore auto-generated code

The list above is created afterwards so it's not exhaustive. I also remove lots of unused files to simplify the project.

What I propose

Copy source code of tylingsoft/dagre-layout to override dagrejs/dagre, and all of us continue the development on dagrejs/dagre.

Why?

Overall I think tylingsoft/dagre-layout is way more modernized. Yarn, ES6, webpack, Jest ... etc.

Concerns

There are some opinionated changes. For example, I removed PhantomJS because it is discontined. I also removed bower support because bower seems to be discontinued too.

Originally I created tylingsoft/dagre-layout for my personal usage and didn't intend to contribute it back. So I might need to make some changes to make everybody accept it.

[cpettitt]

Happy to see the enthusiasm and I'd love to pull changes upstream. I would suggest that we do this as focused patches.

W.r.t. the changesets, some comments:

1. Upgrade all the dependencies (loadash 3 => 4)

+1

2. Yarn instead of NPM

I'm willing to do this if someone is demonstrably willing to support this. In other words, potentially down the road, but definitely not the first patch set. I don't personally have the time to invest in a new package manager.

3. Get rid of PhantomJS

+1.

4. ES6 instead of ES5

I would be fine making the switch to ES6 provided we can transpile to reasonable ES5 for IE11.

5. Use webpack instead of browserify

This is another case where what is there seems good enough. Happy to discuss further the benefits we'd get from this switch. This would also require someone committed to supporting it.

6. Use JavaScript Standard Style instead of JSHint

OK, provided we get some clear benefit.

7. Use Jest for testing

Not a big fan or rewriting all of the tests. If there is a way to support the mocha tests in Jest and migrate over time that would be more palatable.

8. Add test coverage report

We currently have this with Istanbul. What do you propose changing here?

9. Remove Bower support

I believe we still have folks using this. If there is a better solution for distributing the library for the web, I'd be open to potentially replacing this.

10. Use 0 instead of Number.NEGATIVE_INFINITY

Depends on the context. Willing to look at the changes.

11. Git ignore auto-generated code

+1 for this in general. We do need to include the dist files currently to support bower.

[GordonSmith]

My 2c as a consumer of the library:

Upgrade all the dependencies (loadash 3 => 4)

No opinion

Yarn instead of NPM

Latest version of NPM includes a lock file which negates the main benefit of yarn.

Get rid of PhantomJS

+1 - I switched to chrome headless from PhontomJS without too much pain.

ES6 instead of ES5

+1 - With es5 support via babel or equivalent (can Babel generate TypeScript type/definition files?). If it was myself I would switch to TypeScript altogether.

Use webpack instead of browserify

+1 - The current bundling never "just worked" for me.

Use JavaScript Standard Style instead of JSHint

No opinion

Use Jest for testing

No opinion

Add test coverage report

No opinion

Remove Bower support

+1 - I dropped my bower usage a while back (and would have thought >95% of folks are now using npm), plus npm can pull direct from GitHub thus emulating bower support.

Use 0 instead of Number.NEGATIVE_INFINITY

No opinion

Git ignore auto-generated code

+1 (on principle)

[lutzroeder]

@tylerlong can you create a separate issue for each of those items so we can track and discuss them separately? Added #233 for the last one already.

[tylerlong]

Hi guys,

Thank you for the feedback.

For all the items that I propose, I have already done them in https://github.com/tylingsoft/dagre-layout, so there is no "who is going to do the hard work?" concern. The only concern is: if we don't accept all of them, I might need to revert those we don't accept.

Let me explain each of them in detail

1. Upgrade all the dependencies (including loadash 3 => 4)

It's always a good idea to upgrade all the dependencies to the latest stable version. Because new versions of libraries contain improvements and bug fixes. And it's a good idea to be up-to-date.

2. Yarn instead of NPM

I use Yarn for its speed. It's normally faster than NPM. And it is very similar to NPM. Just remember to replace "npm install" with "yarn add" and you are good to go.

With above being said, I agree that there is nothing that Yarn could do but NPM couldn't. So it's not a must-have change.

3. Get rid of PhantomJS

The reason is pretty convincing: PhantomJS has been discontinued. It's dangerous to depend on something abandoned.

4. ES6 instead of ES5

Just to stay up-to-date. And we will build ES5 distribution files for compatibility. And it is common practice to do so. And we won't have issue to support IE 11.

5. Use webpack instead of browserify

Nowadays webpack is way more popular than browserify. I would choose browserify if it's 2014, but it's 2018.

Check this google trends page: https://trends.google.com/trends/explore?date=today%205-y&q=webpack,browserify

6. Use JavaScript Standard Style instead of JSHint

JavaScript Standard style is opinionated. But JSHint belongs to 5 years ago. Most people use ESLint now (JavaScript Standard style is based on ESLint)

7. Use Jest for testing

Not a big fan or rewriting all of the tests. If there is a way to support the mocha tests in Jest and migrate over time that would be more palatable.

I have already done the hard work. Please check https://github.com/tylingsoft/dagre-layout/tree/master/test

Just clone the project, run "yarn install" followed by "yarn test" to confirm that all the tests pass.

8. Add test coverage report

We currently have this with Istanbul. What do you propose changing here?

I mean this badge:

And it is powered by Jest. We can use Jest for both testing and coverage and it's one more reason to use Jest.

9. Remove Bower support

Please google search is bower dead

People can continue to use the already published bower packages. We just don't release new versions to bower any more.

I don't get the point of bower. NPM is a much better option. If you don't want to use NPM, you should use CDN. All the NPM packages have free CDN: https://unpkg.com/dagre/

10. Use 0 instead of Number.NEGATIVE_INFINITY

I don't remember why I changed it(maybe I thought it's shorter to type and it seems to be elegant). But I have done the change and it just worked.

Depends on the context. Willing to look at the changes.

Please check https://github.com/tylingsoft/dagre-layout. Search for NEGATIVE_INFINITY and your will get 0 result. And search this project to see where the NEGATIVE_INFINITYs are and compare what I have changed.

11. Git ignore auto-generated code

I guess nobody is against this idea. And if people want prebuilt content, here it is https://unpkg.com/dagre@0.8.2/dist/ (We do publish prebuilt content to NPM and all the NPM packages have free CDN)

[tylerlong]

@lutzroeder @cpettitt I do think it is necessary to create separate issues.

But my overall suggestion is to apply all of them as a whole instead of applying them one by one.

Because

https://github.com/tylingsoft/dagre-layout was forked in Sep, 2017 and it's not long ago. I have done much more work(I already posted a list of changes above and it is not exhaustive) in tylingsoft/dagre-layout than you have done here since Sep, 2017.

There are two options:

1. I apply all the changes from tylingsoft/dagre-layout to this project, one by one.
2. Override this project's source code with tylingsoft/dagre-layout, and revert we don't agree on.

Option 2 is much easier.

Please

Evaluate https://github.com/tylingsoft/dagre-layout and see if it is in a good and modern status. And please think about: is it OK to override this project' code base with it? Personally I am pretty confident in tylingsoft/dagre-layout.

[cpettitt]

I would like to try this with option 1, for two reasons:

1. Rewriting history upstream will break any current clones of the repository
2. It gives us a chance to review each change in context

I also like Lutz's proposal to split this out into separate tickets so we can have more focused discussion.

[tylerlong]

Understood. It's a community project so we should make everybody on the same page.

[tylerlong]

I will try to create separate issues today. But I am not able to implement them recently because it is Chinese New Year 🇨🇳 🎉 . I will take a 10+ days holiday from tomorrow on.

[cpettitt]

No problem - we can pick them up when you have time. Happy Chinese New Year!

[gabrielgrant]

@tylerlong did you ever get a chance to get back to this? would be great to get some of these changed merged upstream