rollup-config-added #34

Foxtrot-14 · 2023-10-02T19:42:11Z

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 · 2023-10-18T18:52:41Z

rollup.config.js

+const createUmd = process.env.CREATE_UMD === "true";
+const input = "src/index.js"; // Update with your entry point
+const outputDir = "build"; // Update with your desired output directory
+const name = "Cytposcape";


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

rollup.config.js

maxkfranz · 2023-11-07T14:04:45Z

Let's resolve all the comments.
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 · 2023-11-11T08:59:03Z

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

maxkfranz · 2023-11-15T23:32:49Z

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

> [email protected] 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 · 2023-11-15T23:33:35Z

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 · 2023-11-19T18:24:33Z

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

Foxtrot-14 · 2023-11-25T20:30:05Z

Please check.

maxkfranz · 2023-11-27T13:53:51Z

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 · 2023-12-06T13:08:00Z

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 · 2023-12-06T15:45:16Z

The demo is this repo is an example of globals.

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

Foxtrot-14 · 2023-12-06T18:25:39Z

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 · 2023-12-06T19:00:56Z

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 · 2023-12-07T23:45:24Z

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

maxkfranz · 2023-12-11T19:24:08Z

@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 · 2023-12-12T22:25:36Z

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 · 2023-12-13T17:22:50Z

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 · 2023-12-24T10:17:42Z

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 · 2023-12-24T17:17:19Z

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 · 2023-12-25T05:34:39Z

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 · 2023-12-27T19:52:39Z

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

maxkfranz · 2024-01-10T20:52:31Z

Notes:

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.
ESM tests should import cytoscape.esm.min.js.
The node tests should have something logged to the console so you can confirm it actually ran.
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 · 2024-01-15T07:29:01Z

All four points have been addressed.

Foxtrot-14 · 2024-01-24T20:42:27Z

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 · 2024-01-31T18:52:17Z

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

Foxtrot-14 · 2024-02-05T19:04:44Z

@maxkfranz Please merge the PR.

maxkfranz · 2024-02-14T20:59:48Z

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 · 2024-02-15T14:04:47Z

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 · 2024-02-20T22:11:28Z

@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?

rollup-config-added #34

rollup-config-added #34

Conversation

Foxtrot-14 commented Oct 2, 2023 • edited Loading

Summary:

Changes Made:

Testing:

maxkfranz left a comment

Choose a reason for hiding this comment

maxkfranz Oct 18, 2023

Choose a reason for hiding this comment

maxkfranz commented Nov 7, 2023

Foxtrot-14 commented Nov 11, 2023

maxkfranz commented Nov 15, 2023

maxkfranz commented Nov 15, 2023

Foxtrot-14 commented Nov 19, 2023 • edited Loading

Foxtrot-14 commented Nov 25, 2023

maxkfranz commented Nov 27, 2023 • edited Loading

Foxtrot-14 commented Dec 6, 2023 • edited Loading

maxkfranz commented Dec 6, 2023

Foxtrot-14 commented Dec 6, 2023

maxkfranz commented Dec 6, 2023

Foxtrot-14 commented Dec 7, 2023

maxkfranz commented Dec 11, 2023

Foxtrot-14 commented Dec 12, 2023

maxkfranz commented Dec 13, 2023 • edited Loading

Foxtrot-14 commented Dec 24, 2023

maxkfranz commented Dec 24, 2023

Foxtrot-14 commented Dec 25, 2023 • edited Loading

maxkfranz commented Dec 27, 2023

maxkfranz commented Jan 10, 2024

Foxtrot-14 commented Jan 15, 2024

Foxtrot-14 commented Jan 24, 2024 • edited Loading

Foxtrot-14 commented Jan 31, 2024

Foxtrot-14 commented Feb 5, 2024 • edited Loading

maxkfranz commented Feb 14, 2024

Foxtrot-14 commented Feb 15, 2024

maxkfranz commented Feb 20, 2024

Foxtrot-14 commented Feb 21, 2024

maxkfranz commented Feb 26, 2024

Foxtrot-14 commented Feb 27, 2024

maxkfranz commented Feb 28, 2024

Foxtrot-14 commented Feb 29, 2024

maxkfranz commented Mar 5, 2024

Foxtrot-14 commented Mar 7, 2024

Foxtrot-14 commented Mar 21, 2024

stale bot commented Apr 21, 2024

Foxtrot-14 commented Apr 21, 2024

Foxtrot-14 commented May 3, 2024

Foxtrot-14 commented Oct 2, 2023 •

edited

Loading

Foxtrot-14 commented Nov 19, 2023 •

edited

Loading

maxkfranz commented Nov 27, 2023 •

edited

Loading

Foxtrot-14 commented Dec 6, 2023 •

edited

Loading

maxkfranz commented Dec 13, 2023 •

edited

Loading

Foxtrot-14 commented Dec 25, 2023 •

edited

Loading

Foxtrot-14 commented Jan 24, 2024 •

edited

Loading

Foxtrot-14 commented Feb 5, 2024 •

edited

Loading