pagedjs icon indicating copy to clipboard operation
pagedjs copied to clipboard

For discussion: css-tree update

Open janheise opened this issue 1 year ago • 9 comments

This PR is just for discussion as the changes do not result in a building pagedjs

The changes in the first commit combined with a css-tree version of 2.3.1 results, if copied into the dependencies of our project, successful renderings.

The second commit was an attempt to get the build and docker based tests running but with no success.

janheise avatar Jun 27 '24 11:06 janheise

You can solve the first error with:

diff --git a/rollup.config.js b/rollup.config.js
index aed74a2..0d1f254 100644
--- a/rollup.config.js
+++ b/rollup.config.js
@@ -5,7 +5,7 @@ import terser from "@rollup/plugin-terser";
 import license from "rollup-plugin-license";
 import nodePolyfills from 'rollup-plugin-polyfill-node';
 
-import pkg from "./package.json" assert {
+import pkg from "./package.json" with {
   type: 'json'
 };
 

Then, there is an issue with css-tree that exports an ESM module but that ESM modules uses Node specific features making it incompatible with the browser and rollup (the bundler we use)

See:

  • https://github.com/csstree/csstree/issues/278
  • https://github.com/rollup/rollup/issues/4274
  • https://github.com/eight04/rollup-plugin-cjs-es/issues/24

mildred avatar Aug 23 '24 19:08 mildred

Someone got the same issue with css-tree and rollup : https://github.com/rollup/rollup/issues/4597

Suggestion is to avoid bundling css-tree and instead require it at runtime:

I'd suggest externalize it. The best you can try is to bundle using preserveModules, preserveEntrySignatures and copy those binaries on writeBundle. (See https://github.com/cawa-93/vite-electron-builder/pull/904)

We could also switch from rollup to webpack as it seems webpack supports createRequire.

Or we could fork css-tree and remove this nasty createRequire

mildred avatar Aug 23 '24 19:08 mildred

css-tree already has some code to remove the createRequire: https://github.com/csstree/csstree/blob/ba6dfd8bb0e33055c05f13803d04825d98dd2d8d/scripts/esm-to-cjs.cjs#L60

mildred avatar Aug 23 '24 19:08 mildred

I could generate a build with:

diff --git a/package-lock.json b/package-lock.json
index 084abc5..31a6fd3 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -36,6 +36,7 @@
         "playwright-core": "^1.32.3",
         "rimraf": "^5.0.0",
         "rollup": "^3.20.6",
+        "rollup-plugin-includepaths": "^0.2.4",
         "rollup-plugin-license": "^3.0.1",
         "rollup-plugin-livereload": "^2.0.5",
         "rollup-plugin-node-builtins": "^2.1.2",
@@ -9536,6 +9537,13 @@
         "fsevents": "~2.3.2"
       }
     },
+    "node_modules/rollup-plugin-includepaths": {
+      "version": "0.2.4",
+      "resolved": "https://registry.npmjs.org/rollup-plugin-includepaths/-/rollup-plugin-includepaths-0.2.4.tgz",
+      "integrity": "sha512-iZen+XKVExeCzk7jeSZPJKL7B67slZNr8GXSC5ROBXtDGXDBH8wdjMfdNW5hf9kPt+tHyIvWh3wlE9bPrZL24g==",
+      "dev": true,
+      "license": "MIT"
+    },
     "node_modules/rollup-plugin-inject": {
       "version": "3.0.2",
       "resolved": "https://registry.npmjs.org/rollup-plugin-inject/-/rollup-plugin-inject-3.0.2.tgz",
@@ -17670,6 +17678,12 @@
         "fsevents": "~2.3.2"
       }
     },
+    "rollup-plugin-includepaths": {
+      "version": "0.2.4",
+      "resolved": "https://registry.npmjs.org/rollup-plugin-includepaths/-/rollup-plugin-includepaths-0.2.4.tgz",
+      "integrity": "sha512-iZen+XKVExeCzk7jeSZPJKL7B67slZNr8GXSC5ROBXtDGXDBH8wdjMfdNW5hf9kPt+tHyIvWh3wlE9bPrZL24g==",
+      "dev": true
+    },
     "rollup-plugin-inject": {
       "version": "3.0.2",
       "resolved": "https://registry.npmjs.org/rollup-plugin-inject/-/rollup-plugin-inject-3.0.2.tgz",
diff --git a/package.json b/package.json
index 5035118..0747059 100644
--- a/package.json
+++ b/package.json
@@ -43,6 +43,7 @@
     "playwright-core": "^1.32.3",
     "rimraf": "^5.0.0",
     "rollup": "^3.20.6",
+    "rollup-plugin-includepaths": "^0.2.4",
     "rollup-plugin-license": "^3.0.1",
     "rollup-plugin-livereload": "^2.0.5",
     "rollup-plugin-node-builtins": "^2.1.2",
diff --git a/rollup.config.js b/rollup.config.js
index aed74a2..0d41495 100644
--- a/rollup.config.js
+++ b/rollup.config.js
@@ -4,14 +4,29 @@ import json from "@rollup/plugin-json";
 import terser from "@rollup/plugin-terser";
 import license from "rollup-plugin-license";
 import nodePolyfills from 'rollup-plugin-polyfill-node';
+import includePaths from 'rollup-plugin-includepaths';
 
-import pkg from "./package.json" assert {
+import pkg from "./package.json" with {
   type: 'json'
 };
 
 const plugins = [
-	nodeResolve({
-		extensions: [".cjs",".mjs", ".js"]
+  //nodeResolve({
+  //  // force css-tree to load CommonJS as ESM use NodeJS specific features
+	//	extensions: [".cjs"],
+	//	mainFields: ["main"],
+	//	resolveOnly: ["css-tree"]
+	//}),
+	includePaths({
+	  // css-tree unbundled ESM uses NodeJS specific features
+    include: {
+      'css-tree': 'node_modules/css-tree/dist/csstree.esm.js'
+    },
+    extensions: ['.js']
+  }),
+  nodeResolve({
+		extensions: [".cjs",".mjs", ".js"],
+		resolveOnly: module => module != "css-tree"
 	}),
 	commonjs({
 		include: "node_modules/**",

mildred avatar Aug 23 '24 20:08 mildred

The new version with ne css-tree seems to work a bit, some selectors that were ignored before are no longer, however, the whole style from my book is completely off (pages don't even have the correct size any more)

mildred avatar Aug 23 '24 20:08 mildred

The issue I had was something specific with other patches I had.

How, I have an error funcNode.children.last() is not a function, I had to change last() to last but now I have an infinite loop when paging the content (it starts again on the first page every other 10 page with a overflow message on the console (I never had overflows before)

mildred avatar Aug 23 '24 20:08 mildred

There is an issue with the id transformation to data-id in stylesheets, but I still have infinite looping due to overflows

There should not be overflows in the first place, but the infinite looping is still an issue. It is caused by chunker/layout.js where on overflow the findBreakToken() function is called (in checkOverflowAfterResize() in chunker/page.js) with a null node (the argument is omitted and null is the default value). Thus the resulting break token has a null node. When resuming the rendering, getStart() is called on that break token and because the node is null, it starts from the beginning, resulting in an infinite loop.

edit: this occurs when I have a manual break

mildred avatar Aug 24 '24 11:08 mildred

The infinite loop is corrected by setting in findBreakToken() (chunker/layout.js) the following line:

		if (node === null) node = prevBreakToken?.node

edit: except all of the overflow is lost...

edit again: infinite looping is probably caused by 6c8cf0c4b0df1f244e51c33208da42e558544ad6 which changes how the break token is created when node is null. The fix above is probably not right.

mildred avatar Aug 24 '24 11:08 mildred

Thanks for getting started with this @mildred, getting the new css tree in is definitely needed. @fchasen not sure you’ve seen this one.

julientaq avatar Aug 25 '24 13:08 julientaq