Skip to content

Commit

Permalink
Javascript -- v1.2.0: Reimplement ems transpilation, and swap to it a…
Browse files Browse the repository at this point in the history
…s the default. (#479)

* Add a node-options loader to use esm with mocha
* very WIP, writing about this ridiculous ecosystem
* demo to assert imports of the project work
* the project is buildable and the demo runs!
* use esm tsconf for docs
* Update setup-node action
* Change the demo test to use the new npm script names
* Add cross-env to let the test script continue to work on windows
* Add eol spec to gitattr
* Just don't bother with verifying the lib checkin on the matrix of runners, once in quick is enough
---------
* set "type": "module" and swap default main and types to the esm build.
---------
Co-authored-by: Skenvy <>
  • Loading branch information
Skenvy authored Mar 17, 2024
1 parent 1151fca commit e318518
Show file tree
Hide file tree
Showing 57 changed files with 327 additions and 274 deletions.
3 changes: 3 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,6 @@
# our GitHub Actions, which would otherwise change the line ending and fail.
*.js text eol=lf
*.ts text eol=lf
*.cjs text eol=lf
*.mjs text eol=lf
package.json text eol=lf
8 changes: 4 additions & 4 deletions .github/workflows/javascript-build.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ jobs:
- name: 🏁 Checkout
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: 🟨🟦🟩🟥 Set up Node
uses: actions/setup-node@8c91899e586c5b171469028077307d293428b516 # v3.5.1
uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2
with:
node-version-file: 'javascript/.nvmrc'
cache: 'npm'
Expand Down Expand Up @@ -116,7 +116,7 @@ jobs:
name: Package
path: javascript/
- name: 🟨🟦🟩🟥 Set up Node
uses: actions/setup-node@8c91899e586c5b171469028077307d293428b516 # v3.5.1
uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2
with:
node-version-file: 'javascript/.nvmrc'
registry-url: 'https://npm.pkg.github.com'
Expand All @@ -142,7 +142,7 @@ jobs:
name: Package
path: javascript/
- name: 🟨🟦🟩🟥 Set up Node
uses: actions/setup-node@8c91899e586c5b171469028077307d293428b516 # v3.5.1
uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2
with:
node-version-file: 'javascript/.nvmrc'
registry-url: 'https://registry.npmjs.org'
Expand All @@ -165,7 +165,7 @@ jobs:
- name: 🏁 Checkout
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: 🟨🟦🟩🟥 Set up Node
uses: actions/setup-node@8c91899e586c5b171469028077307d293428b516 # v3.5.1
uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2
with:
node-version-file: 'javascript/.nvmrc'
cache: 'npm'
Expand Down
27 changes: 13 additions & 14 deletions .github/workflows/javascript-test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ jobs:
- name: 🏁 Checkout
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: 🟨🟦🟩🟥 Set up Node
uses: actions/setup-node@8c91899e586c5b171469028077307d293428b516 # v3.5.1
uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2
with:
node-version-file: 'javascript/.nvmrc'
cache: 'npm'
Expand Down Expand Up @@ -62,7 +62,7 @@ jobs:
- name: 🏁 Checkout
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: 🟨🟦🟩🟥 Set up Node ${{ matrix.version }}
uses: actions/setup-node@8c91899e586c5b171469028077307d293428b516 # v3.5.1
uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2
with:
node-version: '${{ matrix.version }}'
cache: 'npm'
Expand All @@ -75,10 +75,9 @@ jobs:
run: make test
- name: 🧹 Lint
run: make lint
# I'm not sure what benefit there is to doing this check in the whole matrix
# but it'd be interesting if it shook something loose I didn't expect!
- name: ⚖ Does the checked in JS match the transpiled TS?
run: make verify_transpiled_checkin
# We probably don't need to do this on all runners, and the windows CRLF echos are annoying
# - name: ⚖ Does the checked in JS match the transpiled TS?
# run: make verify_transpiled_checkin
built-example:
name: JavaScript 🟨🟦🟩🟥 Build Example 🦛
if: >-
Expand All @@ -89,7 +88,7 @@ jobs:
- name: 🏁 Checkout
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: 🟨🟦🟩🟥 Set up Node
uses: actions/setup-node@8c91899e586c5b171469028077307d293428b516 # v3.5.1
uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2
with:
node-version-file: 'javascript/.nvmrc'
cache: 'npm'
Expand Down Expand Up @@ -119,7 +118,7 @@ jobs:
- name: 🏁 Checkout
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: 🟨🟦🟩🟥 Set up Node
uses: actions/setup-node@8c91899e586c5b171469028077307d293428b516 # v3.5.1
uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2
with:
node-version-file: 'javascript/.nvmrc'
- name: 🆒 Download dists
Expand All @@ -128,11 +127,11 @@ jobs:
name: Package
path: javascript/.demo
- name: 🦛 Install this package
run: npm run strip-version && npm install
- name: 🦏 Transpile demo
run: npm run build
- name: 🦏 Run usage demonstration
run: npm run demo
run: npm run setup
- name: 🦏 Transpile and run demonstration; 🦑 CJS
run: npm run everything:cjs
- name: 🦏 Transpile and run demonstration; 🐙 ESM
run: npm run everything:esm
codeql:
name: JavaScript 🟨🟦🟩🟥 CodeQL 🛡👨‍💻🛡
if: >-
Expand All @@ -152,7 +151,7 @@ jobs:
- name: 🏁 Checkout
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: 🟨🟦🟩🟥 Set up Node
uses: actions/setup-node@8c91899e586c5b171469028077307d293428b516 # v3.5.1
uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2
with:
node-version-file: 'javascript/.nvmrc'
cache: 'npm'
Expand Down
2 changes: 2 additions & 0 deletions javascript/.demo/.gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
package-lock.json
_/*.js
_/*.mjs
_/*.cjs
1 change: 0 additions & 1 deletion javascript/.demo/_/stoppingTime.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import * as Collatz from '@skenvy/collatz';


// Default (P,a,b); 0 trap [as b is not a multiple of a]
// 'should return the Default (P,a,b); 0 trap'
// // Test 0's immediated termination.
Expand Down
27 changes: 19 additions & 8 deletions javascript/.demo/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,29 @@
"license": "Apache-2.0",
"main": "index.js",
"dependencies": {
"@skenvy/collatz": "file:./skenvy-collatz.tgz"
"@skenvy/collatz": "file:skenvy-collatz.tgz"
},
"scripts": {
"clean": "rm **/*.js",
"clean:modules": "rm -rf node_modules",
"clean:js": "rm -f _/**.js",
"clean:cjs": "rm -f _/**.cjs",
"clean:esm": "rm -f _/**.mjs",
"clean": "npm run clean:modules && npm run clean:js && npm run clean:cjs && npm run clean:esm",
"strip-version": "[ -f skenvy-collatz.tgz ] || mv skenvy-collatz-*.tgz skenvy-collatz.tgz",
"build": "tsc --module CommonJS --target ES2020 --lib ES2020 --types node ./_/*.ts",
"demo": "find ./_/*.js -maxdepth 1 -type f | xargs -L1 node",
"everything": "npm run clean && npm run strip-version && npm i && npm run build && npm run demo"
"build:cjs": "tsc --module CommonJS --moduleResolution node --target ES2020 --lib ES2020 --types node ./_/*.ts",
"build:esm": "tsc --module ESNext --moduleResolution node --target ES2022 --lib ES2022 --types node ./_/*.ts",
"ftype:cjs": "for f in `find ./_ -iname '*.js' -type f -print`;do mv \"$f\" ${f%.js}.cjs; done",
"ftype:esm": "for f in `find ./_ -iname '*.js' -type f -print`;do mv \"$f\" ${f%.js}.mjs; done",
"demo:cjs": "find ./_/*.cjs -maxdepth 1 -type f | xargs -L1 node",
"demo:esm": "find ./_/*.mjs -maxdepth 1 -type f | xargs -L1 node",
"setup": "npm run clean && npm run strip-version && npm i -f && npm i file:skenvy-collatz.tgz",
"everything:cjs": "npm run build:cjs && npm run ftype:cjs && npm run demo:cjs && npm run clean:cjs",
"everything:esm": "npm run build:esm && npm run ftype:esm && npm run demo:esm && npm run clean:esm",
"everything": "npm run setup && npm run everything:cjs && npm run everything:esm"
},
"devDependencies": {
"@types/node": "^20.3.2",
"async": "^3.2.4",
"typescript": "^5.1.5"
"@types/node": "^20.11.28",
"async": "^3.2.5",
"typescript": "^5.4.2"
}
}
4 changes: 3 additions & 1 deletion javascript/.mocharc.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
{
"extension": ["ts"],
"spec": "./**/*.spec.ts",
"require": "ts-node/register"
"require": "ts-node/register",
"loader": "ts-node/esm",
"es-module-specifier-resolution": "node"
}
14 changes: 11 additions & 3 deletions javascript/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ NPM=$(_) npm
INSTALL_NODE=$(NVM) install $$(cat ./.nvmrc)
INSTALL_NPM=$(NPM) i -g npm@$$(cat ./.npm-version)
endif
.PHONY: bump_major bump_minor bump_patch install_node install_npm setup update clean docs docs_server test lint verify_transpiled_checkin build publish g
PKG_NAME=skenvy-collatz
.PHONY: bump_major bump_minor bump_patch install_node install_npm setup update clean docs docs_server test lint verify_transpiled_checkin build publish demo _demo
SHELL:=/bin/bash

# Versions can be manually changed where they appear in the package json and
Expand Down Expand Up @@ -52,7 +53,7 @@ update:
$(NPM) install

clean:
rm -f skenvy-collatz-*.tgz
rm -f $(PKG_NAME)-*.tgz
rm -rf docs
rm -rf .nyc_output
$(NPM) run clean
Expand Down Expand Up @@ -87,4 +88,11 @@ build: clean test lint verify_transpiled_checkin

# https://docs.npmjs.com/cli/v8/commands/npm-publish
publish:
$(NPM) publish --access=public skenvy-collatz-*.tgz
$(NPM) publish --access=public $(PKG_NAME)-*.tgz

demo: build
mv $(PKG_NAME)-*.tgz .demo/$(PKG_NAME).tgz
cd .demo && npm run everything

_demo: # packless demo to rerun it while manually tweaking it
cd .demo && npm run everything
35 changes: 35 additions & 0 deletions javascript/devlog.md
Original file line number Diff line number Diff line change
Expand Up @@ -241,3 +241,38 @@ Well, at least the webhook worked, and it should deploy to deno on the next tag.
# NVM
[NVM](https://github.com/nvm-sh/nvm), "Node version manager", is an extremely useful tool for managing multiple versions of node installed at the same time.
Coming back in here to say that, while updating the supported engines to remove 14 and 16 and add 20, that I'm surprised I hadn't mentioned nvm in here before.
## Another exercise in absurdity.
Yet again, a tumble down a rabbit-worm-hole has transpired from very little shaking the node tree. A dependabot PR to suggest upgrading `chai` from `v4` to `v5` led to quite a while trying to investigate why it was difficult to run `mocha` tests with it, to learn that [chai v5 dropped cjs support](https://github.com/chaijs/chai/issues/1597), and that [mocha's support for esm is still expiremental](https://github.com/mochajs/mocha/issues/4900) [[2:GH](https://github.com/mochajs/mocha-examples/issues/47)] [[3:SO](https://stackoverflow.com/questions/40635956/overriding-tsconfig-json-for-ts-node-in-mocha)]. Also see [this SO RE 'TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts"'](https://stackoverflow.com/questions/71893906/run-mocha-with-typescript-throws-typeerror-err-unknown-file-extension-unknow). The rabbit-worm-hole also involved trying to figure out if our use of `nyc` had any influence / impact on this error, see [[1](https://github.com/istanbuljs/nyc/issues/1287)] [[2](https://github.com/istanbuljs/esm-loader-hook#readme)].

After deciding it would probably just be best to give up on it and say we're sticking with chai v4, I had a look in to the compiled "esm" output that changed last year when setting the `esm` `tsconfig`'s `module` and `moduleResolution` to `nodenext` last year to let through a typescript update. But it would appear now that I should have looked closer at the change in the compiled "esm" code back then, as this change to "nodenext" also changed the output of the "esm" build to "cjs". So the output at the moment is just _two copies of cjs_.

We definitely want to support esm, as the primary target. We might be able to support it optionally, and getting back to that would be a priority (as well as adding a check for this working in the demo, which was only checking that the cjs result was valid), but it would be nice if we could jump straight to esm as our default. We can edit the `package.json` to include the changes;
```diff
@@ -27,8 +27,9 @@
"files": [
"./lib/**/*"
],
- "main": "./lib/cjs/index.js",
- "types": "./lib/cjs/types/index.d.ts",
+ "main": "./lib/esm/index.mjs",
+ "types": "./lib/esm/types/index.d.ts",
+ "type": "module",
"exports": {
".": {
"import": {
```
But this still yeilds a `TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /<>/Collatz/javascript/tests/collatzFunction.spec.ts` during the `nyc mocha` execution. Following up with specifying `TS_NODE_PROJECT='./tsconfig.esm.json'` before the `nyc mocha` gets the same error. If we go ahead and add the `.mocharc.json` differences recommended by [a comment at the end of this](https://github.com/mochajs/mocha/issues/4900);
```diff
@@ -1,7 +1,5 @@
{
"extension": ["ts"],
"spec": "./**/*.spec.ts",
- "require": "ts-node/register"
+ "require": "ts-node/register",
+ "loader": "ts-node/esm",
+ "es-module-specifier-resolution": "node"
}
```
We get the error `TS2835: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean '../src/index.js'`. Addressing this by adding `.ts` in all relative imports in the tests yields a different error `TS5097: An import path can only end with a '.ts' extension when 'allowImportingTsExtensions' is enabled.`. Adding the requested `"allowImportingTsExtensions": true` to our base `tsconfig` now yields another 10 or so `TS2835: Relative import paths need explicit file extensions in ECMAScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean './XYZ.js'?`. Doing the same thing with all relative imports in the src yields passing tests, but the subsequent `tsc` gives us `npm ERR! error TS5096: Option 'allowImportingTsExtensions' can only be used when either 'noEmit' or 'emitDeclarationOnly' is set.`.

Whilst googling to try and solve this often yielded very thoroughly answered SO posts such as [this one about modules and moduleResolution](https://stackoverflow.com/questions/71463698/why-we-need-nodenext-typescript-compiler-option-when-we-have-esnext), the overall wholistic answer came from [here](https://dev.to/ayc0/typescript-50-new-mode-bundler-esm-1jic). The gist being that typescript wont ever change the name of a module, which includes not renaming a relative import of a `.ts` file to a `.js` file, but the relative link will only have any meaning in the context of having already been transpiled, so the suggestion to rename things to `.js` is not as misleading as it seemed initially, which did a lot of heavy lifting the bury the lead. But yes, relative imports with `.js`, for what they will be when they are transpiled, lets it work! However we still need the relative links to `.ts` files in our mocha spec files. So we can get around both of these by having a new tsconfig that extends our ems set of options, and add the necessary `allowImportingTsExtensions` setting to that one, so that mocha sees it's allowed to import `.ts`, our `tsc` can still emit `ems` code, and wont complain that it can't emit anything with `allowImportingTsExtensions` set. Now it's just a matter of getting the demo to be happy with both import and require.
24 changes: 12 additions & 12 deletions javascript/lib/cjs/HailstoneSequence.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.hailstoneSequence = exports.HailstoneSequence = void 0;
const utilities_1 = require("./utilities");
const function_1 = require("./function");
const utilities_js_1 = require("./utilities.js");
const function_js_1 = require("./function.js");
/** Contains the results of computing a hailstone sequence. */
class HailstoneSequence {
/**
Expand All @@ -20,17 +20,17 @@ class HailstoneSequence {
* Thrown if either P or a are 0.
*/
constructor(initialValue, P, a, b, maxTotalStoppingTime, totalStoppingTime) {
this.terminate = (0, utilities_1.stoppingTimeTerminus)(initialValue, totalStoppingTime);
this.terminate = (0, utilities_js_1.stoppingTimeTerminus)(initialValue, totalStoppingTime);
if (initialValue === 0n) {
// 0 is always an immediate stop.
this.values = [0n];
this.terminalCondition = utilities_1.SequenceState.ZERO_STOP;
this.terminalCondition = utilities_js_1.SequenceState.ZERO_STOP;
this.terminalStatus = 0;
}
else if (initialValue === 1n) {
// 1 is always an immediate stop, with 0 stopping time.
this.values = [1n];
this.terminalCondition = utilities_1.SequenceState.TOTAL_STOPPING_TIME;
this.terminalCondition = utilities_js_1.SequenceState.TOTAL_STOPPING_TIME;
this.terminalStatus = 0;
}
else {
Expand All @@ -39,16 +39,16 @@ class HailstoneSequence {
this.values = [initialValue];
let next;
for (let k = 1; k <= minMaxTotalStoppingTime; k += 1) {
next = (0, function_1.collatzFunction)({ n: this.values[k - 1], P: P, a: a, b: b });
next = (0, function_js_1.collatzFunction)({ n: this.values[k - 1], P: P, a: a, b: b });
// Check if the next hailstone is either the stopping time, total
// stopping time, the same as the initial value, or stuck at zero.
if (this.terminate(next)) {
this.values.push(next);
if (next === 1n) {
this.terminalCondition = utilities_1.SequenceState.TOTAL_STOPPING_TIME;
this.terminalCondition = utilities_js_1.SequenceState.TOTAL_STOPPING_TIME;
}
else {
this.terminalCondition = utilities_1.SequenceState.STOPPING_TIME;
this.terminalCondition = utilities_js_1.SequenceState.STOPPING_TIME;
}
this.terminalStatus = k;
return;
Expand All @@ -62,19 +62,19 @@ class HailstoneSequence {
break;
}
}
this.terminalCondition = utilities_1.SequenceState.CYCLE_LENGTH;
this.terminalCondition = utilities_js_1.SequenceState.CYCLE_LENGTH;
this.terminalStatus = cycleInit;
return;
}
if (next === 0n) {
this.values.push(0n);
this.terminalCondition = utilities_1.SequenceState.ZERO_STOP;
this.terminalCondition = utilities_js_1.SequenceState.ZERO_STOP;
this.terminalStatus = -k;
return;
}
this.values.push(next);
}
this.terminalCondition = utilities_1.SequenceState.MAX_STOP_OUT_OF_BOUNDS;
this.terminalCondition = utilities_js_1.SequenceState.MAX_STOP_OUT_OF_BOUNDS;
this.terminalStatus = minMaxTotalStoppingTime;
}
}
Expand All @@ -96,7 +96,7 @@ exports.HailstoneSequence = HailstoneSequence;
*/
function hailstoneSequence({ initialValue, P = 2n, a = 3n, b = 1n, maxTotalStoppingTime = 1000, totalStoppingTime = true }) {
// Call out the function before any magic returns to trap bad values.
const throwaway = (0, function_1.collatzFunction)({ n: initialValue, P: P, a: a, b: b });
const throwaway = (0, function_js_1.collatzFunction)({ n: initialValue, P: P, a: a, b: b });
// Return the hailstone sequence.
return new HailstoneSequence(initialValue, P, a, b, maxTotalStoppingTime, totalStoppingTime);
}
Expand Down
4 changes: 2 additions & 2 deletions javascript/lib/cjs/TreeGraph.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"use strict";
Object.defineProperty(exports, "__esModule", { value: true });
exports.treeGraph = exports.TreeGraph = void 0;
const TreeGraphNode_1 = require("./TreeGraphNode");
const TreeGraphNode_js_1 = require("./TreeGraphNode.js");
/**
* Contains the results of computing the Tree Graph via Collatz.treeGraph(~).
* Contains the root node of a tree of TreeGraphNode's.
Expand All @@ -19,7 +19,7 @@ class TreeGraph {
* Thrown if either P or a are 0.
*/
constructor(nodeValue, maxOrbitDistance, P, a, b) {
this.root = TreeGraphNode_1.TreeGraphNode.new(nodeValue, maxOrbitDistance, P, a, b);
this.root = TreeGraphNode_js_1.TreeGraphNode.new(nodeValue, maxOrbitDistance, P, a, b);
}
}
exports.TreeGraph = TreeGraph;
Expand Down
Loading

0 comments on commit e318518

Please sign in to comment.