rollup-config-added

[Foxtrot-14]

Pull Request Description

Summary:

This pull request addresses the issue 223 on NRNB's official repository for GSOC issues.

Changes Made:

• Rollup config file added and a test prototype developed for the extension.
• A template repository is made, to provide a convention for the extentions to be built going furthur.

Testing:

• The prototype is tested in HTML, React, NPM.
• The test files are in a repository.

Additional Comments:

• The experience of working on this feature has been of great value. The patience and honesty of the reviewer is very much appriciated.

[maxkfranz]

Great start.

Let’s address these points below:

[maxkfranz]

This ought to be the package name of the extension, i.e. cytoscape-automove.

[maxkfranz]

1. Let's resolve all the comments.
2. Publishing your fork to something like @foxtrot14/cytoscape-automove-esm-test on npm would help to expediently validate that you've updated everything correctly.

[Foxtrot-14]

I have published the package on npm test. Please let me know what should be my next move.

[maxkfranz]

Some notes re. cytoscape.js-automove-foxtrot:

It looks like the new build files are not included in your package. See:

foxtrot-test % ls node_modules/cytoscape.js-automove-foxtrot
LICENSE                 bower.json              demo-multiple-mean.html package.json            src
README.md               cytoscape-automove.js   demo.html               rollup.config.js        webpack.config.js

The npm script targets for watch and dev should be updated. You can delete dev if needed, but there needs to be a watch:

    "watch": "webpack --progress --watch",
    "dev": "webpack-dev-server --open",

The build target should be replaced to use rollup.

"build": "cross-env NODE_ENV=production webpack",

The dependencies or the build configuration look like they need to be updated.

From a clean node_modules, after running npm install, npm run build:rollup fails to complete:

cytoscape.js-automove % npm run build:rollup

> cytoscape-automove@1.10.3 build:rollup
> rollup -c


src/index.js → dist/cytoscape-automove.umd.min.js...
babelHelpers: 'bundled' option was used by default. It is recommended to configure this option explicitly, read more here: https://github.com/rollup/plugins/tree/master/packages/babel#babelhelpers
[!] (plugin commonjs--resolver) RollupError: Rollup requires that your Babel configuration keeps ES6 module syntax intact. Unfortunately it looks like your configuration specifies a module transformer to replace ES6 modules with another module format. To continue you have to disable it.

Most commonly it's a CommonJS transform added by @babel/preset-env - in such case you should disable it by adding `modules: false` option to that preset (described in more detail here - https://github.com/rollup/plugins/tree/master/packages/babel#modules ).
src/index.js
    at error (/Users/max/Documents/workspace/cytoscape.js-automove/node_modules/rollup/dist/shared/rollup.js:353:30)
    at Object.error (/Users/max/Documents/workspace/cytoscape.js-automove/node_modules/rollup/dist/shared/rollup.js:1721:20)
    at Object.error (/Users/max/Documents/workspace/cytoscape.js-automove/node_modules/rollup/dist/shared/rollup.js:25627:42)
    at preflightCheck (file:///Users/max/Documents/workspace/cytoscape.js-automove/node_modules/@rollup/plugin-babel/dist/es/index.js:83:9)
    at file:///Users/max/Documents/workspace/cytoscape.js-automove/node_modules/@rollup/plugin-babel/dist/es/index.js:296:13
    at transformCode (file:///Users/max/Documents/workspace/cytoscape.js-automove/node_modules/@rollup/plugin-babel/dist/es/index.js:124:24)
    at transform (/Users/max/Documents/workspace/cytoscape.js-automove/node_modules/rollup/dist/shared/rollup.js:25604:16)
    at ModuleLoader.addModuleSource (/Users/max/Documents/workspace/cytoscape.js-automove/node_modules/rollup/dist/shared/rollup.js:25804:30)

[maxkfranz]

Note:

max@macbook cytoscape.js-automove % node --version
v16.19.0
max@macbook cytoscape.js-automove % npm --version
8.19.3
max@macbook cytoscape.js-automove %

[Foxtrot-14]

All the point above have been addressed and a fresh package has been published...fingers crossed.

[Foxtrot-14]

Please check.

[maxkfranz]

Let's expedite the review with some testing homework for you:

(1) Create a new node project, i.e. npm init, and verify you can import and require your package. Both should work.

(2) Create a react project using the create-react-project scaffolder. Import your package. Verify it works. Do the same for require.

(3) Create a new html file. Use import to pull in your package. Verify it works with no build system.

(4) Do the same as (3) for another html file, this time with the UMD build. Verify it works with old-fashioned globals.

You can put these tests on jsbin or codebin so that other people can inspect them afterwards.

Looking forward to your next update.

[Foxtrot-14]

Hello, the import works fine no errors encountered. But, I am unable to get the cytoscape variable to use an extension. Could you please provide a code snippet where the extension is used both in vanilla HTML and React. Other than that the package is ready to be used.

[maxkfranz]

The demo is this repo is an example of globals.

For react, you can search github and find lots of examples.

[Foxtrot-14]

Ok, the html file works well. For react i could find examples that used cytoscape but i couldn't find examples using extensions along with them.

[maxkfranz]

but i couldn't find examples using extensions along with them.

Search for a particular extension and react, rather than cytoscape and react. In any case, it's just the import pattern rather than the globals one.

[Foxtrot-14]

Tests done, I tried using JSBin or CodePen but looked like it would be better to post these on github itself.

[maxkfranz]

@Foxtrot-14, here is some feedback regarding your current test files:

HTML

• Good. This tests both UMD builds.
• Remaining: There should also be a test file that uses the ESM build. Browsers support ESM natively now, and this test file should use that approach.
• General: The test files should be named according to what each one tests, e.g. test-umd.html.

NPM:

• Each of the tests, require and UMD, should verify that the extension has been registered successfully. You can test for whether cy.automove exists, for instance.

React:

• The React test looks like a good start. However, it does not exercise the extension. Like the NPM tests, you should verify that the extension has been successfully registered. Most importantly, the demo graph in the test file should use the extension, like the HTML test files. Someone should be able to open up the React file once it's built, and verify that the extension is functioning correctly (e.g. by dragging nodes around).

Looking forward to your updates

[Foxtrot-14]

OK,
HTML: files are updated one tests the UMD build and the other tests the ESM build.
NPM: I tried printing the cy.automove object it prints "undefined", however if I print cytoscape and automove separately I get function register value for both. Kinda stuck here.
React: the react project does implement the extension if you go to the App.js component on line 6 you will find Cytoscape.use(automove);, I did get a the graph when i ran the project, maybe the extension can not be seen in action because there are no closed loops, perhaps if you could help me with the npm issue this will not be tough to tackle.

[maxkfranz]

NPM: I tried printing the cy.automove object it prints "undefined", however if I print cytoscape and automove separately I get function register value for both. Kinda stuck here.

You should use an assertion, not a console statement.

E.g.

assert(cy.automove != null)
assert(typeof cy.automove === typeof function(){})

You can think of other, more specific assertions to confirm further.

React: the react project does implement the extension if you go to the App.js component on line 6 you will find Cytoscape.use(automove);, I did get a the graph when i ran the project, maybe the extension can not be seen in action because there are no closed loops, perhaps if you could help me with the npm issue this will not be tough to tackle.

use()ing the extension is not enough. You need to set the rules, like the main demo in this repo.

[Foxtrot-14]

Ok, I have set rules in the react project and it works fine, for the npm part, I get assertion failed for cytoscape.automove but it works fine if I assert them individually.

[maxkfranz]

It should be cy.automove as the reference (i.e. on the instance), not cytoscape.automove -- where cytoscape() is the constructor. If you want to directly test the prototype/class, you need to do something like cytoscape.prototype.automove or getPrototypeOf(cy).automove.

[Foxtrot-14]

Ok, both(import and require) assertions have passed, and HTML files are also working fine. Please check the react project and let me know if it works well. Happy Holidays.

[maxkfranz]

I'll look into the update soon, and I'll post feedback in this thread.

[maxkfranz]

Notes:

1. The original post at the top of this thread should be updated to include a more detailed description of the changes and links to the test package and the test repo for reference.
2. ESM tests should import cytoscape.esm.min.js.
3. The node tests should have something logged to the console so you can confirm it actually ran.
4. The network in the React demo should be the same as the HTML demo, again to confirm that it's actually working as expected.

[Foxtrot-14]

All four points have been addressed.

[Foxtrot-14]

Hello, thank you for your patience and insight throughout the whole process, I intend to finish my work on this issue by the end of March, and solve issue 235 as my GSOC 2024 internship. So, the solution of this issue will add more weight on my proposal overall. Please assign the issue to me so I can work on all the extensions by March.
Kind Regards,
Noaman

[Foxtrot-14]

Hello, if everything is fine, then please let me know so I can start working on all the extensions one by one.

[Foxtrot-14]

@maxkfranz Please merge the PR.

[maxkfranz]

I started trying out your tests: https://github.com/Foxtrot-14/Test-automove.git

The npm-import case is failing:

npm install
node index.js
Assertion failed: Not Null

The npm-require example is failing in the same way.

I didn't bother trying the React tests, since the npm cases are failing.

Please update your tests and verify that all cases are working as expected. I'll review once that's complete.

[Foxtrot-14]

The assertion was to check if the object was null, the assertion failing means that the object is not null, If you were to check the react project you'd see that the extension is working fine in there. Anyway I have updated the assertion for better understanding.
Waiting for your response.

[maxkfranz]

@Foxtrot-14

I've added some comments, attached to the code.

Assertions should be like this:

assert(whatIThinkShouldBeTrue);

console.log('Everything worked OK, otherwise the above line would throw an error.');

Make sense?

See also:

• Improving ESM support and documentation, esp. for browsers cytoscape.js#3217

[Foxtrot-14]

I don't understand. Added comments where exactly...?

[maxkfranz]

https://github.com/cytoscape/cytoscape.js-automove/pull/34/files

[Foxtrot-14]

Ok the assertion syntax is fixed. Let me know what more is to be done.

[maxkfranz]

What about all the other comments in the review?

[Foxtrot-14]

All are old and already addressed.

[maxkfranz]

All of the remaining comment threads are unaddressed.

Only the addressed ones have been closed.

[Foxtrot-14]

I just hadn't marked them as resolved. I have read all the comments and resolved them as well.

[Foxtrot-14]

If all is good, please merge the PR.

[stale]

This issue has been automatically marked as stale, because it has not had activity within the past 30 days. It will be closed if no further activity occurs within the next 30 days. If a feature request is important to you, please consider making a pull request. Thank you for your contributions.

[Foxtrot-14]

What about all the other comments in the review?

All the issues have been addressed.

[Foxtrot-14]

I wanna finish what I started regardless of whether I got into GSoC or not.