package.json: script dist:zip packs with "katex" directory prefix again

[ccorn]

This addresses #1664. yarn dist:zip now creates a temporary copy of dist to katex and tars/zips that folder instead of just its contents. This restores the release structure before v0.10.0-rc.

Caveat: I am not familiar with npm nor yarn (beyond the basics) nor the KaTeX build system. I am familiar with Unix shell scripting however. Please review this pull request thoroughly. There may be other approaches that fit your philosophy or design goals better.

Details: set -e makes the shell exit on errors (in simple commands); this saves lots of && and turns the test ! -d katex into an assertion. The (...) open a subshell. This might not be necessary if yarn uses a separate shell process for each script anyway, but I use parentheses as part of an idiom whenever I use trap. (Because it enables nesting, and with the parentheses you can try the script directly in an interactive bash session.) That trap registers cleanup commands, to be run whenever (and however) the subshell exits, i.e. at the ) or upon errors (because of the set -e) or signals as sent by hitting Ctrl-C. In this case, the trap makes sure that the temporary katex folder is deleted on exit, even in case of errors.

The patch makes yarn dist produce katex.tar.gz and katex.zip, each containing a single (filled) folder katex. All five test suites invoked by yarn test pass.

I had to use yarn upgrade and my yarn.lock differs, but I have not included those changes because I think those are platform-specific.

[khanbot]

CLA signature looks good 👍

[codecov]

Codecov Report

Merging #1665 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1665   +/-   ##
=======================================
  Coverage   93.84%   93.84%           
=======================================
  Files          78       78           
  Lines        4568     4568           
  Branches      801      801           
=======================================
  Hits         4287     4287           
  Misses        248      248           
  Partials       33       33

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5585187...52a6696. Read the comment docs.

[ylemkimon]

We aim NPM scripts to be cross-platform, including Windows, as possible. Therefore unfortunately, we cannot use POSIX features. Also please use rimraf instead of rm.

[ccorn]

Hmm. It seems I cannot make this both cross-platform and as noninvasive as I'd like. Therefore I withdraw this pull request.

[ylemkimon]

I think we can just do: cp -R dist katex && tar czf katex.tar.gz katex && rimraf katex.zip && zip -rq katex.zip katex && rimraf katex and add katex to .gitignore.

[ccorn]

Note that cp -R dist katex presumes that no directory katex exists. Otherwise, the command does not fail; it produces wrong results instead. Which is why my proposal tested for that. You might want to prepend a rimraf katex && at least.

[ccorn]

That said, beginning with an unconditional rimraf katex might violate the principle that one should only delete stuff that is known to be autogenerated. You know the build system better than I, so I won't be the judge of that.
Thinking about it, you might actually want to begin with rimraf katex katex.tar.gz katex.zip to make sure that the archives are not leftovers from an ancient successful yarn dist:zip whilst later runs have aborted early.

[ccorn]

Besides, is the cp -R actually cross-platform enough? If not,

mkdirp katex && tar cf - -C dist . | tar xf - -C katex

might do the trick.

[edemaine]

My guess would be that, if someone has tar, they also have cp -R, though I'm not certain. (Personally I run Linux or Windows+Cygwin. I'm not sure how someone would normally get tar on Windows, without Cygwin or Mintty...)

[followup] And I think rimraf katex before cp makes sense.

[ccorn]

I do not know rollup but it seems its output directory can be configured. What about using dist/katex for that?

[edemaine]

@ccorn The issue is that we need the files in dist for other reasons. We used to do it something like that, and #1500 aimed to clean up the scripts to focus on building into dist, which we need much more often than zipping. Copying the files from dist into dist/katex could be cool, but then we can't use cp -R (I think).

[ccorn]

The following seems to work. This configures the output directory for rollup and webpack directly. No temporary copies necessary. However, I do not really know the tools; I have just skimmed their configuration. Please take a look.

diff --git a/package.json b/package.json
index c54e8ac..5ab317d 100644
--- a/package.json
+++ b/package.json
@@ -87,10 +87,10 @@
     "prestart": "node check-node-version.js && yarn check && node src/unicodeMake.js",
     "start": "webpack-dev-server --hot --config webpack.dev.js",
     "analyze": "webpack --config webpack.analyze.js",
-    "build": "yarn prestart && rimraf dist/ && mkdirp dist && cp README.md dist && rollup -c && webpack",
+    "build": "yarn prestart && rimraf dist/ && mkdirp dist/katex && cp README.md dist/katex && rollup -c && webpack",
     "watch": "yarn build --watch",
     "dist": "yarn test && yarn build && yarn dist:zip",
-    "dist:zip": "cd dist && tar czf ../katex.tar.gz * && zip -rq ../katex.zip *"
+    "dist:zip": "rimraf katex.tar.gz katex.zip && cd dist && tar czf ../katex.tar.gz katex && zip -rq ../katex.zip katex"
   },
   "dependencies": {
     "commander": "^2.16.0"
diff --git a/rollup.config.js b/rollup.config.js
index 092c3c4..3fd7708 100644
--- a/rollup.config.js
+++ b/rollup.config.js
@@ -5,7 +5,7 @@ process.env.BABEL_ENV = 'esm';
 export default {
     input: 'katex.js',
     output: {
-        file: 'dist/katex.mjs',
+        file: 'dist/katex/katex.mjs',
         format: 'es',
     },
     plugins: [
diff --git a/webpack.common.js b/webpack.common.js
index 3815bc6..42c0e88 100644
--- a/webpack.common.js
+++ b/webpack.common.js
@@ -70,7 +70,7 @@ function createConfig(target /*: Target */, dev /*: boolean */,
             // Enable output modules to be used in browser or Node.
             // See: https://github.com/webpack/webpack/issues/6522
             globalObject: "(typeof self !== 'undefined' ? self : this)",
-            path: path.resolve(__dirname, 'dist'),
+            path: path.resolve(__dirname, 'dist/katex'),
             publicPath: dev ? '/' : '',
         },
         module: {

[ccorn]

@edemaine: Sorry, I have noticed your answer too late. Well then. The direct way is not an option then.

[ccorn]

Out of curiosity, since yarn test passes with the above diff: What would it actually break?

[ccorn]

Ah I see, the node_modules/katex/dist/katex.js shall be preserved. Then copying dist to katex seems to be the right approach after all.

[ccorn]

Please do not wait for a pull request from me. In most KaTeX matters, I feel like a bull in a china shop. You would have to review it anyway, and it's probably easier for you to write the line yourself.

[ylemkimon]

That said, beginning with an unconditional rimraf katex might violate the principle that one should only delete stuff that is known to be autogenerated. You know the build system better than I, so I won't be the judge of that.

I wonder there is anyone who has a file or folder named katex on the KaTeX project folder, who're running KaTeX dist script, so I think it'd be OK to delete it before running script. (We're doing same for dist folder.)

Thinking about it, you might actually want to begin with rimraf katex katex.tar.gz katex.zip to make sure that the archives are not leftovers from an ancient successful yarn dist:zip whilst later runs have aborted early.

Thank you for pointing that out. I didn't know zip adds files to existing zip file.

I do not know rollup but it seems its output directory can be configured. What about using dist/katex for that?

Out of curiosity, since yarn test passes with the above diff: What would it actually break?

Unfortunately, package.json main field, CLI, local testing pages, and NPM-published package depend on this path, so it's be more breaking change to change this path.

[ylemkimon]

Could you change the dist:zip script to:

rimraf katex/ katex.tar.gz katex.zip && cp -R dist katex && tar czf katex.tar.gz katex && zip -rq katex.zip katex && rimraf katex/

and add /katex/ to the .gitignore?

[ccorn]

Deliberately not adding /katex/ to .gitignore because if katex/ shows up in git status output, that indicates that the latest yarn dist:zip attempt has not completed successfully, which I consider useful information. (Remember that katex/ gets removed in case of success.)

[ccorn]

The above commit line uses the exact script proposal by @ylemkinon.
In the context of the considerations here, that seems appropriate to me.
The essential assertions about the original proposal also hold here:

The commit makes yarn dist produce katex.tar.gz and katex.zip, each containing a single (filled) folder katex. All five test suites invoked by yarn test pass.

I had to use yarn upgrade and my yarn.lock differs, but I have not included those changes because I think those are platform-specific.

[ylemkimon]

Could you merge latest master branch or allow maintainers edit?

[ccorn]

Rebased (and retested with yarn dist which includes yarn test).

[ylemkimon]

Thank you!