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

Package manager CI action #1113

Merged
merged 22 commits into from
Sep 7, 2021
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
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
97 changes: 97 additions & 0 deletions .github/workflows/package-manager-ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
name: package-manager-ci
on:
push:
paths-ignore:
- 'docs/**'
- '*.md'
pull_request:
paths-ignore:
- 'docs/**'
- '*.md'
jobs:
pnpm:
name: pnpm package manager on ${{ matrix.node-version }} ${{ matrix.os }}
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
os: [macOS-latest, windows-latest]
Eomm marked this conversation as resolved.
Show resolved Hide resolved
node-version: [12, 14, 16]
steps:
- uses: actions/[email protected]
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/[email protected]
with:
node-version: ${{ matrix.node-version }}
- name: Use pnpm
uses: pnpm/[email protected]
with:
version: ^6.0.0
- name: Install dependancies
run: pnpm install
- name: Tests
run: pnpm run test-ci

yarn:
name: yarn package manager on ${{ matrix.node-version }} ${{ matrix.os }}
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
os: [macOS-latest]
Eomm marked this conversation as resolved.
Show resolved Hide resolved
node-version: [12, 14, 16]
steps:
- uses: actions/[email protected]
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/[email protected]
with:
node-version: ${{ matrix.node-version }}
- name: Use yarn
run: |
npm install -g yarn
Eomm marked this conversation as resolved.
Show resolved Hide resolved
yarn set version berry && yarn set version 2.4.2
Eomm marked this conversation as resolved.
Show resolved Hide resolved
echo "nodeLinker: node-modules" >> .yarnrc.yml
# see https://github.com/yarnpkg/berry/issues/2935#issuecomment-911299992
yarn add --dev typescript@~4.3.2
Comment on lines +49 to +50
Copy link

Choose a reason for hiding this comment

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

Suggested change
# see https://github.com/yarnpkg/berry/issues/2935#issuecomment-911299992
yarn add --dev typescript@~4.3.2

We've released the fix for this issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

To avoid this issue for future typescripts minor releases, where could we read the last working version integrated?

Copy link

@merceyz merceyz Sep 7, 2021

Choose a reason for hiding this comment

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

We don't have it documented anywhere, we try to update the patch as soon as they make a new release. Also TypeScript doesn't follow semver so their minor is actually a major release

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TypeScript doesn't follow semver so their minor is actually a major release

For this reason, the pino's maintainer may evaluate to pin the dep to ~4.4.0 so it will be dependabot to update it and trigger the new error for new minor changes

yarn install
env:
# needed due the yarn.lock file in pino's .gitignore
YARN_ENABLE_IMMUTABLE_INSTALLS: false
- name: Tests
run: yarn run test-ci

yarn-pnp:
name: yarn-pnp package manager on ${{ matrix.node-version }} ${{ matrix.os }}
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
os: [macOS-latest]
Eomm marked this conversation as resolved.
Show resolved Hide resolved
node-version: [12, 14, 16]
steps:
- uses: actions/[email protected]
- name: Use Node.js ${{ matrix.node-version }}
uses: actions/[email protected]
with:
node-version: ${{ matrix.node-version }}
- name: Use yarn
run: |
npm install -g yarn
Eomm marked this conversation as resolved.
Show resolved Hide resolved
yarn set version berry
echo 'nodeLinker: pnp
packageExtensions:
debug@*:
dependencies:
supports-color: "*"
treport@*:
dependencies:
tap-yaml: "*"
' >> .yarnrc.yml
yarn add --dev typescript@~4.3.2
Copy link

Choose a reason for hiding this comment

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

Suggested change
yarn add --dev typescript@~4.3.2

yarn install
yarn add --dev transport@link:./test/fixtures/transport
Copy link

Choose a reason for hiding this comment

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

I meant you should do this regardless of what is running and get rid of the manual symlinking

Copy link
Member

Choose a reason for hiding this comment

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

doing the same with npm across multiple node version is not that easy.

Copy link

Choose a reason for hiding this comment

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

With npm you can use the file: protocol which has been supported since 2.0.0
https://docs.npmjs.com/cli/v7/configuring-npm/package-json#local-paths

env:
# needed due the yarn.lock file in pino's .gitignore
YARN_ENABLE_IMMUTABLE_INSTALLS: false
- name: Tests
run: yarn run test-ci-yarn-pnp
6 changes: 6 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,12 @@ build/Release
# Dependency directory
node_modules

# yarn cache
.yarn
.yarnrc.yml
.pnp
.pnp.*

# Optional npm cache directory
.npm

Expand Down
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
"lint": "eslint .",
"test": "npm run lint && tap test/*test.js test/*/*test.js && npm run test-types",
"test-ci": "npm run lint && tap --no-check-coverage test/*test.js test/*/*test.js --coverage-report=lcovonly && npm run test-types",
"test-ci-yarn-pnp": "npm run lint && tap --no-check-coverage test/*test.js test/*/*test.js --coverage-report=lcovonly",
Eomm marked this conversation as resolved.
Show resolved Hide resolved
"test-types": "tsc && tsd && ts-node test/types/pino.ts",
"cov-ui": "tap --coverage-report=html test/*test.js test/*/*test.js",
"bench": "node benchmarks/utils/runbench all",
Expand Down Expand Up @@ -81,6 +82,7 @@
"import-fresh": "^3.2.1",
"log": "^6.0.0",
"loglevel": "^1.6.7",
"pino-elasticsearch": "^6.1.0",
"pino-pretty": "pinojs/pino-pretty#master",
"pre-commit": "^1.2.2",
"proxyquire": "^2.1.3",
Expand Down Expand Up @@ -108,7 +110,7 @@
"pino-std-serializers": "^4.0.0",
"quick-format-unescaped": "^4.0.3",
"sonic-boom": "^2.2.1",
"thread-stream": "^0.11.0"
"thread-stream": "^0.11.1"
},
"tsd": {
"directory": "test/types"
Expand Down
6 changes: 4 additions & 2 deletions test/esm/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
const t = require('tap')
const semver = require('semver')

if (!semver.satisfies(process.versions.node, '^13.3.0 || ^12.10.0 || >= 14.0.0')) {
const { isYarnPnp } = require('../helper')

if (!semver.satisfies(process.versions.node, '^13.3.0 || ^12.10.0 || >= 14.0.0') || isYarnPnp) {
t.skip('Skip esm because not supported by Node')
} else {
// Node v8 throw a `SyntaxError: Unexpected token import`
Expand All @@ -17,7 +19,7 @@ if (!semver.satisfies(process.versions.node, '^13.3.0 || ^12.10.0 || >= 14.0.0')
})
}

if (!semver.satisfies(process.versions.node, '>= 14.13.0 || ^12.20.0')) {
if (!semver.satisfies(process.versions.node, '>= 14.13.0 || ^12.20.0') || isYarnPnp) {
t.skip('Skip named exports because not supported by Node')
} else {
// Node v8 throw a `SyntaxError: Unexpected token import`
Expand Down
3 changes: 2 additions & 1 deletion test/helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const pid = process.pid
const hostname = os.hostname()

const isWin = process.platform === 'win32'
const isYarnPnp = process.versions.pnp !== undefined

function getPathToNull () {
return isWin ? '\\\\.\\NUL' : '/dev/null'
Expand Down Expand Up @@ -72,4 +73,4 @@ function watchFileCreated (filename) {
})
}

module.exports = { getPathToNull, sink, check, once, sleep, watchFileCreated, isWin }
module.exports = { getPathToNull, sink, check, once, sleep, watchFileCreated, isWin, isYarnPnp }
73 changes: 54 additions & 19 deletions test/transport.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const { join } = require('path')
const { once } = require('events')
const { readFile, symlink, unlink } = require('fs').promises
const { test } = require('tap')
const { isWin, watchFileCreated } = require('./helper')
const { isWin, isYarnPnp, watchFileCreated } = require('./helper')
const pino = require('../')
const url = require('url')
const strip = require('strip-ansi')
Expand All @@ -15,6 +15,32 @@ const writer = require('flush-write-stream')
const { pid } = process
const hostname = os.hostname()

async function installTransportModule () {
try {
await uninstallTransportModule()
} catch {}
try {
await symlink(
join(__dirname, 'fixtures', 'transport'),
join(__dirname, '..', 'node_modules', 'transport')
)
} catch (err) {
if (!isYarnPnp) {
Eomm marked this conversation as resolved.
Show resolved Hide resolved
throw err
}
}
}

async function uninstallTransportModule () {
try {
await unlink(join(__dirname, '..', 'node_modules', 'transport'))
} catch (err) {
if (!isYarnPnp) {
Eomm marked this conversation as resolved.
Show resolved Hide resolved
throw err
}
}
}

test('pino.transport with file', async ({ same, teardown }) => {
const destination = join(
os.tmpdir(),
Expand Down Expand Up @@ -53,21 +79,14 @@ test('pino.transport with package', { skip: isWin }, async ({ same, teardown })
'_' + Math.random().toString(36).substr(2, 9)
)

try {
await unlink(join(__dirname, '..', 'node_modules', 'transport'))
} catch {}

await symlink(
join(__dirname, 'fixtures', 'transport'),
join(__dirname, '..', 'node_modules', 'transport')
)
await installTransportModule()

const transport = pino.transport({
target: 'transport',
options: { destination }
})
teardown(async () => {
await unlink(join(__dirname, '..', 'node_modules', 'transport'))
await uninstallTransportModule()
transport.end()
})
const instance = pino(transport)
Expand All @@ -83,6 +102,29 @@ test('pino.transport with package', { skip: isWin }, async ({ same, teardown })
})
})

test('pino.transport loads the pino-elasticsearch package', ({ plan, ok, fail, equal, pass }) => {
plan(3)
const transport = pino.transport({
target: 'pino-elasticsearch',
options: {
node: null // triggers the error if the module is loaded correctly
}
})

const instance = pino(transport)
instance.info('hello')

transport.on('ready', function () {
fail('ready event should not be emitted')
})
transport.on('error', function (err) {
ok(err)
equal(err.message, 'Missing node(s) option', 'pino-elastisearch trigger a valid error')
})

pass('pino instance created')
})
Eomm marked this conversation as resolved.
Show resolved Hide resolved

test('pino.transport with file URL', async ({ same, teardown }) => {
const destination = join(
os.tmpdir(),
Expand Down Expand Up @@ -297,14 +339,7 @@ test('pino.transport with package as a target', { skip: isWin }, async ({ same,
'_' + Math.random().toString(36).substr(2, 9)
)

try {
await unlink(join(__dirname, '..', 'node_modules', 'transport'))
} catch {}

await symlink(
join(__dirname, 'fixtures', 'transport'),
join(__dirname, '..', 'node_modules', 'transport')
)
await installTransportModule()

const transport = pino.transport({
targets: [{
Expand All @@ -313,7 +348,7 @@ test('pino.transport with package as a target', { skip: isWin }, async ({ same,
}]
})
teardown(async () => {
await unlink(join(__dirname, '..', 'node_modules', 'transport'))
await uninstallTransportModule()
transport.end()
})
const instance = pino(transport)
Expand Down