Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typescript, Rollup, Jest, Terser, Rimraf, announcer, de-binarying, CI platforming #107

Closed
wants to merge 22 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
a4cad5c
Adds node 16; builds on win/mac/linux; removes pnpm; adds npm install…
StoneCypher Apr 22, 2021
9d2a92b
Temporarily de-check 10.x
StoneCypher Apr 22, 2021
6007b8f
compiling
StoneCypher Apr 23, 2021
a26c3d5
rollup and typescript
StoneCypher Apr 23, 2021
e060a31
terser
StoneCypher Apr 23, 2021
7cccf9a
Also Jest, and a version incrementer
StoneCypher Apr 23, 2021
9979d55
oh and coverage
StoneCypher Apr 23, 2021
a4c4d9a
merge prior to squash
StoneCypher Apr 27, 2021
b558583
post-merge
StoneCypher Apr 27, 2021
5dfe18f
remove binary executable status of benchmark/run, commute to run.js
StoneCypher Apr 27, 2021
211242c
re-involve peggy; complete
StoneCypher Apr 27, 2021
6d79b2f
uh, does gha need the extension?
StoneCypher Apr 27, 2021
3fb5e27
it ... it couldn't be a name conflict?
StoneCypher Apr 27, 2021
244c3c4
removing benchmark from ci to see what happens
StoneCypher Apr 27, 2021
854d823
remove vestigial test claim from action
StoneCypher Apr 27, 2021
b25b9df
remove coverage from repo (prs too annoying, will do in action in art…
StoneCypher Apr 27, 2021
5bd8083
Add announcer; update changelog; deposit minified file
StoneCypher Apr 27, 2021
7a9e97f
regress version bump (version dent?) from 1.2.0 back to 1.1.0
StoneCypher Apr 28, 2021
8a67d82
Fix many linting errors. Move version script to tools/set_version.js…
StoneCypher Apr 28, 2021
c32aac6
last step cleanups
StoneCypher Apr 29, 2021
4f49a1d
modernize chai and sinon
StoneCypher Apr 29, 2021
fde3ed8
missed linting after moving set_version; fixed
StoneCypher Apr 29, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 6 additions & 22 deletions .github/workflows/node.js.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
---
name: Tests

on:
Expand All @@ -11,33 +10,18 @@ on:
jobs:
build:

runs-on: ubuntu-20.04

strategy:
matrix:
node-version: [10.x, 12.x, 14.x, 15.x]
node-version: [10.x, 12.x, 14.x, 15.x, 16.x]
os: [ubuntu-latest, windows-latest, macos-latest]

runs-on: ${{ matrix.os }}

steps:
- uses: actions/checkout@v2
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v1
with:
node-version: ${{ matrix.node-version }}
- name: Cache pnpm modules
uses: actions/cache@v2
env:
cache-name: cache-pnpm-modules
with:
# pnpm cache files are stored in `~/.pnpm-store` on Linux/macOS
path: ~/.pnpm-store
key: ${{ runner.os }}-build-${{ env.cache-name }}-${{ matrix.node-version }}-${{ hashFiles('**/package.json') }}
restore-keys: |
${{ runner.os }}-build-${{ env.cache-name }}-${{ matrix.node-version }}-
- name: Install pnpm
uses: pnpm/action-setup@v1.2.1
with:
version: 5.18.9
run_install: true
- name: Lint ${{ matrix.node-version }}
run: npm run lint
- name: Test ${{ matrix.node-version }}
run: npm run test:bare
run: npm install && npm run build
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be npm ci since we've gone to the trouble of not caching anything, installing all of the dev tools, and the pain of package-lock. This can be fixed later, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reason it is traditional to not use npm ci in a situation like this is that npm ci is a newcomer to the node world - it was only introduced in node 5.7, which is more than halfway into node's lifecycle, and long after it had continuous integration norms.

npm ci is meant to be used when the ci build is different than the local build. when a node programmer sees that npm install && npm run build - the default for their own local setup - is being used to test in ci, they learn quite a bit about the expectations of strength of their local test set.

(i don't actually understand how package-lock is relevant here, so maybe i'm missing something)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm ci requires a package-lock, and I'm still salty about having to maintain what I think of as a useless file (for a package with no dependencies), but I'm willing to do it since there's some potential theoretical benefit to it. I'm looking for any real benefit to maintaining it so I stop feeling salty. :)

Note: that's all about my feelings about myself, and unrelated to the technical solution we pick. It's just background.

We don't have to move to npm ci

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still salty about having to maintain what I think of as a useless file (for a package with no dependencies), but I'm willing to do it since there's some potential theoretical benefit to it. I'm looking for any real benefit to maintaining it so I stop feeling salty. :)

There is a benefit. It's ridiculously niche, but it has burned me once in the real world. There is a second theoretical benefit that I've never seen personally.

 

The thing I've seen

Node sometimes builds non-node services. Those things are frequently built through tools like node-gyp. Those occasionally interact with operating system level package managers.

Back in yon paleozoic era, when we still used things like nightmare to manage browser testing, it was reasonably common to have both nightmare and selenium in place, because you could do most of your acceptance tests in nightmare with 1/10 the effort of selenium, but since that was electron and locked you into current-chrome, you could also do your horror path stuff in selenium and have full browser backversion testing.

selenium requested the cairo (an svg renderer) version in the package manager; electron set a floor, instead. electron had a several years newer version than selenium did.

We were image testing SVG. The version of cairo in use determined whether several things we needed, particularly miter joins on path endcaps, actually worked correctly.

As a result, if you installed electron first, you'd get the version of cairo that had been modern when your chosen electron distro was used, which selenium would happily use

If on the other hand you installed selenium first, you'd get the old version, and then the supporting tools around freetype would cause a version lock (think anaconda requirements,) meaning that when it upgraded, it'd actually get the correct modern version, instead of whatever electron thought was the modern version

We had a significantly obsoleted electron version because one of our customers was struggling to update their SSL ciphers because they in turn had some customers running on like windows negative four or something

Therefore, depending on what order you installed the two non-node things, you'd get significantly different tooling behavior from your underlying support libraries

It took us three days to figure out what the hell was wrong. Once we did, we just installed the libraries before doing node anything, and it went away

However if you're distributing something where a user needs to rely on node-gyp or its demon bretheren, order of construction can be important

 

The thing I haven't seen

Rumor has it an equivalent problem can exist inside of javascript node dependencies.

I've never understood how, or heard a coherent explanation, but the author of babel maintains this to be true, so I believe it.

I have never seen it and cannot explain.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrt npm ci, i'm probably actually going to move to it later, to give you the variant behavior you want for eslint

that's an appropriate time because at that point ci really is behaving differently than local

i was just explaining why i hadn't yet done that

4 changes: 3 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
browser/*
browser/
examples/*.js
node_modules/
pnpm-lock.yaml
yarn.lock
build/
coverage/
2 changes: 2 additions & 0 deletions .npmignore
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,7 @@ src
test
tools
.vscode/
build/
pnpm-lock.yaml
yarn.lock
coverage/
13 changes: 13 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,19 @@ Released: TBD

### Minor Changes

- Infrastructural rebake [@StoneCypher](https://github.com/peggyjs/peggy/pull/107)
- Builds with `typescript`, removes `babel`
- Bundles with `rollup`, removes `webpack`
- Rollup is configured to create two bundles - es6 and umd
- Only umd is exposed; will expose es6 with #119
- Tests with `jest`, removes `mocha`
- Enables coverage analysis using `lcov` notation
- Coverage is not archived; will archive with #120
- Minifies with `terser`, removes `uglify`
- Adds `rimraf` for portable pre-build cleanup
- Extends CI testing to `windows`, `macintosh`
- Increases node testing range to include `node 16`
- Adds an announcer to make the build process more readable
- `location()`s now will have additional `source` property which value is taken
from the `options.grammarSource` property. That property can contain arbitrary
data,for example, path to the currently parsed file.
Expand Down
16 changes: 8 additions & 8 deletions benchmark/run → benchmark/run_bench.js
100755 → 100644
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
#!/usr/bin/env node

"use strict";

/* eslint-env node */

let Runner = require("./runner.js");
let benchmarks = require("./benchmarks.js");
let fs = require("fs");
const Runner = require("./runner.js");
const benchmarks = require("./benchmarks.js");
const fs = require("fs");

// Results Table Manipulation

Expand All @@ -29,7 +28,7 @@ function padRight(text, length) {
}

function center(text, length) {
let padLength = (length - text.length) / 2;
const padLength = (length - text.length) / 2;

return dup(" ", Math.floor(padLength))
+ text
Expand Down Expand Up @@ -100,7 +99,7 @@ function abort(message) {

// Arguments

let args = process.argv.slice(2); // Trim "node" and the script path.
const args = process.argv.slice(2); // Trim "node" and the script path.

function isOption(arg) {
return (/^-/).test(arg);
Expand All @@ -112,12 +111,13 @@ function nextArg() {

// Main

let runCount = 10;
let options = {
const options = {
cache: false,
optimize: "speed"
};

let runCount = 10;

while (args.length > 0 && isOption(args[0])) {
switch (args[0]) {
case "-n":
Expand Down
64 changes: 1 addition & 63 deletions docs/vendor/peggy/peggy.min.js

Large diffs are not rendered by default.

6 changes: 6 additions & 0 deletions lib/version.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@

// This file is generated.
// Do not edit it! Your work will be overwritten.
//
// Instead, please look at ./tools/set_version.js

"use strict";

module.exports = "1.1.0";
Loading