Use Rollup for tests and examples #348

gabalafou · 2022-08-13T11:24:49Z

Closes #332.

Test plan

For each package, for example @lumino/algorithm:

cd packages/algorithm
yarn build:test
yarn test:firefox
yarn test:firefox-headless
yarn test:chrome
yarn test:chrome-headless

One way to check that each package (not examples, not tests) builds exactly the same as before is to build the packages from the main branch, then build them with this PR and compare the two. You can leverage git to do it:

git checkout -b build-from-main main
git clean -xfd
echo node_modules/ > .gitignore
yarn && yarn build && yarn build:examples && yarn build:test
git add .
git commit --no-verify -m"build"
git merge rollup
git checkout -b build-from-rollup rollup
git clean -xfd
echo node_modules/ > .gitignore
yarn && yarn build && yarn build:examples && yarn build:test
git add .
git commit --no-verify -m"build"
git diff build-from-main --stat

You'll see that the bundles for the examples and the tests have changed. That's to be expected because they were previously built with Webpack. But you shouldn't see any differences for the main packages, which were built with Rollup before this PR (and are still built with Rollup after this PR).

welcome · 2022-08-13T11:24:51Z

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

gabalafou · 2022-08-13T11:34:12Z

I'm opening this draft pull request with one package in order to catch any problems before extending to all of the other packages.

I will soon also convert a single example from the examples folder to use rollup instead of webpack.

gabalafou

done self-reviewing

gabalafou · 2022-08-13T11:39:11Z

packages/algorithm/package.json

@@ -26,9 +26,9 @@
    "api": "api-extractor run --local --verbose",
    "build": "npm run build:src && rollup -c",
    "build:src": "tsc --build",
-    "build:test": "tsc --build tests && cd tests && webpack",
+    "build:test": "npm run clean:test && tsc --build tests && cd tests && rollup -c",


Should clean be part of the build step? If you don't run clean and delete the tests/tsconfig.tsbuildinfo file then the build step will do nothing.

gabalafou · 2022-08-13T11:39:41Z

packages/algorithm/package.json

    "clean": "rimraf ./lib && rimraf *.tsbuildinfo && rimraf ./types && rimraf ./dist",
-    "clean:test": "rimraf tests/build",
+    "clean:test": "rimraf tests/lib && rimraf tests/tsconfig.tsbuildinfo",


Must delete the .tsbuildinfo file or else the build command will do nothing.

packages/algorithm/package.json

packages/algorithm/tests/rollup.config.js

packages/algorithm/package.json

gabalafou

Done self-reviewing second commit (example-accordionpanel)

examples/example-accordionpanel/style/index.css

gabalafou · 2022-08-13T14:07:48Z

yarn.lock

@@ -1188,6 +1188,14 @@
    estree-walker "^1.0.1"


All yarn.lock changes should be related to the changes in the package.json files in this PR.

gabalafou · 2022-08-15T11:28:34Z

packages/algorithm/package.json

@@ -26,9 +26,9 @@
    "api": "api-extractor run --local --verbose",
    "build": "npm run build:src && rollup -c",
    "build:src": "tsc --build",
-    "build:test": "tsc --build tests && cd tests && webpack",
+    "build:test": "npm run clean:test && tsc --build tests && cd tests && rollup -c",


I wasn't sure if I should add clean:test here, but without it the build:test script/command is not idempotent because it creates a tsconfig.tsbuildinfo file, which I found prevents a second run of the build:test command from picking up intervening changes in the test code.

gabalafou · 2022-08-15T11:31:03Z

Thanks @blink1073 for reviewing the test-related Rollup changes. I feel confident now extending my changes in @lumino/algorithm tests to the other packages' tests in the monorepo.

fcollonval

Thanks a lot @gabalafou

I added some suggestions before you move forward.

packages/algorithm/package.json

examples/example-accordionpanel/rollup.config.js

fcollonval · 2022-08-16T07:54:27Z

yarn.lock

@@ -7400,6 +7509,13 @@ rollup@^2.77.2:
  optionalDependencies:
    fsevents "~2.3.2"

+rollup@^2.77.3:


Before merging, we should check that this entry disappears as it should merge with the previous one. But it is not a blocker.

Nope. Still hasn't disappeared.

When I added Rollup to the package.json file for each example, it set: "rollup": "^2.77.3".

Would you mind running yarn why rollup to find why we have two versions?

Here is my console log for yarn why rollup:

yarn why rollup rollup yarn why v1.22.19 [1/4] 🤔 Why do we have the module "rollup"...? [2/4] 🚚 Initialising dependency graph... [3/4] 🔍 Finding dependency... [4/4] 🚡 Calculating file sizes... => Found "rollup@2.77.2" info Has been hoisted to "rollup" info Reasons this module exists - "workspace-aggregator-511ef2db-8ac6-4e9e-8474-e5094676397f" depends on it - Hoisted from "_project_#@lumino#algorithm#rollup" - Hoisted from "_project_#@lumino#application#rollup" - Hoisted from "_project_#@lumino#collections#rollup" - Hoisted from "_project_#@lumino#commands#rollup" - Hoisted from "_project_#@lumino#coreutils#rollup" - Hoisted from "_project_#@lumino#datagrid#rollup" - Hoisted from "_project_#@lumino#disposable#rollup" - Hoisted from "_project_#@lumino#domutils#rollup" - Hoisted from "_project_#@lumino#dragdrop#rollup" - Hoisted from "_project_#@lumino#keyboard#rollup" - Hoisted from "_project_#@lumino#messaging#rollup" - Hoisted from "_project_#@lumino#polling#rollup" - Hoisted from "_project_#@lumino#properties#rollup" - Hoisted from "_project_#@lumino#signaling#rollup" - Hoisted from "_project_#@lumino#virtualdom#rollup" - Hoisted from "_project_#@lumino#widgets#rollup" info Disk size without dependencies: "6.43MB" info Disk size with unique dependencies: "6.43MB" info Disk size with transitive dependencies: "6.43MB" info Number of shared dependencies: 0 => Found "@lumino/example-accordionpanel#rollup@2.77.3" info This module exists because "_project_#@lumino#example-accordionpanel" depends on it. info Disk size without dependencies: "6.43MB" info Disk size with unique dependencies: "6.43MB" info Disk size with transitive dependencies: "6.43MB" info Number of shared dependencies: 0 => Found "@lumino/example-datagrid#rollup@2.77.3" info This module exists because "_project_#@lumino#example-datagrid" depends on it. info Disk size without dependencies: "6.43MB" info Disk size with unique dependencies: "6.43MB" info Disk size with transitive dependencies: "6.43MB" info Number of shared dependencies: 0 => Found "@lumino/example-dockpanel#rollup@2.77.3" info This module exists because "_project_#@lumino#example-dockpanel" depends on it. info Disk size without dependencies: "6.43MB" info Disk size with unique dependencies: "6.43MB" info Disk size with transitive dependencies: "6.43MB" info Number of shared dependencies: 0 ✨ Done in 0.85s.

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>

gabalafou · 2022-08-19T13:49:03Z

@fcollonval, could you take a look at my last commit add2f47, is it something like what you had imagined?

One question I have: should I move all of the rollup dependencies (the plugins) out of the various package.jsons and into the root package.json?

gabalafou · 2022-08-19T13:50:16Z

rollup.examples.config.js

+import nodeResolve from '@rollup/plugin-node-resolve';
+import styles from 'rollup-plugin-styles';
+
+const globals = id =>


I copied this line from rollup.src.config because it fixed a warning or error message that I saw from the rollup command.

rollup.src.config.js

packages/algorithm/rollup.config.js

fcollonval · 2022-08-19T14:04:58Z

should I move all of the rollup dependencies (the plugins) out of the various package.jsons and into the root package.json?

@gabalafou it is better to let them in each package where they are used.

gabalafou · 2022-09-05T21:37:43Z

rollup.examples.config.js

+import nodeResolve from '@rollup/plugin-node-resolve';
+import styles from 'rollup-plugin-styles';
+
+export const globals = id =>


There's something I find a bit counter-intuitive about these base config files. For example, I tripped up here because I thought I could replace this line with an import like so:

import { globals } from './rollup.src.config';

But that caused the build script in the examples to fail. That's because importing from that other Rollup config brought along its dependencies, which are not included in the package.json for each of the example packages. In other words, for example, rollup-plugin-postcss is not included as a dependency for any of the example-* packages, but is a dependency in the other packages.

This is expected - a solution would be to define a base config that only uses common dep. But here this is probably not worth the trouble just for the globals definition.

fcollonval

Thanks @gabalafou

We only have to figure out why there are still two versions of rollup in the yarn.lock. And we should be good to go.

fcollonval · 2022-09-06T07:41:20Z

yarn.lock

@@ -7400,6 +7509,13 @@ rollup@^2.77.2:
  optionalDependencies:
    fsevents "~2.3.2"

+rollup@^2.77.3:


Would you mind running yarn why rollup to find why we have two versions?

fcollonval · 2022-09-06T07:47:11Z

rollup.examples.config.js

+import nodeResolve from '@rollup/plugin-node-resolve';
+import styles from 'rollup-plugin-styles';
+
+export const globals = id =>


This is expected - a solution would be to define a base config that only uses common dep. But here this is probably not worth the trouble just for the globals definition.

fcollonval · 2022-09-08T07:50:37Z

Kicking the CI

fcollonval

thanks a lot @gabalafou

gabalafou changed the title ~~Replace webpack with rollup for packages/algorithm/tests~~ Use Rollup for tests and examples Aug 13, 2022

gabalafou mentioned this pull request Aug 13, 2022

Remove webpack from Lumino examples and code base #332

Closed

gabalafou commented Aug 13, 2022

View reviewed changes

Replace webpack with rollup for packages/algorithm/tests

90ad1bf

gabalafou force-pushed the rollup branch from 84089ad to 52cd9d0 Compare August 13, 2022 14:00

Use Rollup with example-accordionpanel

eda3692

gabalafou force-pushed the rollup branch from 52cd9d0 to eda3692 Compare August 13, 2022 14:12

gabalafou commented Aug 13, 2022

View reviewed changes

gabalafou commented Aug 15, 2022

View reviewed changes

blink1073 added the maintenance Dependencies, build, technical debt, etc. label Aug 15, 2022

fcollonval reviewed Aug 16, 2022

View reviewed changes

gabalafou and others added 2 commits August 16, 2022 04:18

Apply suggestions from code review

7bc7876

Co-authored-by: Frédéric Collonval <fcollonval@users.noreply.github.com>

Consolidate rollup configs

add2f47

gabalafou commented Aug 19, 2022

View reviewed changes

gabalafou added 10 commits September 2, 2022 14:58

Merge branch 'main' into rollup

fe677cd

finish adding rollup to examples

88520a2

update createRollupConfig() call

78cc285

call createRollupConfig() from algorithm/test/rollup.config.js

d0ba475

rollup packages/application

4c7d2e2

remove webpack config file from algorithm/tests

1ff2bfb

remove webpack from remaining packages

01406c3

update Yarn lock file

54cf1b6

use base config in coreutils

ec5bd68

fix dependencies in drapdrop and widgets

6768ad1

gabalafou commented Sep 5, 2022

View reviewed changes

gabalafou marked this pull request as ready for review September 5, 2022 21:50

fcollonval reviewed Sep 6, 2022

View reviewed changes

fcollonval added this to the Lumino 2 milestone Sep 6, 2022

fcollonval and others added 2 commits September 8, 2022 09:46

Upgrade rollup

80abac9

Automatic application of license header

6d9cdd1

fcollonval closed this Sep 8, 2022

fcollonval reopened this Sep 8, 2022

github-actions bot assigned gabalafou Sep 8, 2022

fcollonval approved these changes Sep 8, 2022

View reviewed changes

fcollonval merged commit 45a6e9e into jupyterlab:main Sep 8, 2022

github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Rollup for tests and examples #348

Use Rollup for tests and examples #348

gabalafou commented Aug 13, 2022 •

edited

Loading

welcome bot commented Aug 13, 2022

gabalafou commented Aug 13, 2022 •

edited

Loading

gabalafou left a comment

gabalafou Aug 13, 2022

gabalafou Aug 13, 2022

gabalafou left a comment

gabalafou Aug 13, 2022

gabalafou Aug 15, 2022

gabalafou commented Aug 15, 2022

fcollonval left a comment

fcollonval Aug 16, 2022

gabalafou Sep 5, 2022

gabalafou Sep 5, 2022 •

edited

Loading

fcollonval Sep 6, 2022

gabalafou Sep 8, 2022

gabalafou commented Aug 19, 2022 •

edited

Loading

gabalafou Aug 19, 2022

fcollonval commented Aug 19, 2022

gabalafou Sep 5, 2022

fcollonval Sep 6, 2022

fcollonval left a comment

fcollonval Sep 6, 2022

fcollonval Sep 6, 2022

fcollonval commented Sep 8, 2022

fcollonval left a comment

Use Rollup for tests and examples #348

Use Rollup for tests and examples #348

Conversation

gabalafou commented Aug 13, 2022 • edited Loading

Test plan

welcome bot commented Aug 13, 2022

gabalafou commented Aug 13, 2022 • edited Loading

gabalafou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabalafou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabalafou commented Aug 15, 2022

fcollonval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabalafou Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabalafou commented Aug 19, 2022 • edited Loading

Choose a reason for hiding this comment

fcollonval commented Aug 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcollonval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcollonval commented Sep 8, 2022

fcollonval left a comment

Choose a reason for hiding this comment

gabalafou commented Aug 13, 2022 •

edited

Loading

gabalafou commented Aug 13, 2022 •

edited

Loading

gabalafou Sep 5, 2022 •

edited

Loading

gabalafou commented Aug 19, 2022 •

edited

Loading