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 all 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
91 changes: 91 additions & 0 deletions .github/workflows/package-manager-ci.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
name: package-manager-ci
on:
push:
branches:
- main
- package-manager-load
jobs:
pnpm:
name: pnpm package manager on ${{ matrix.node-version }} ${{ matrix.os }}
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
os: [windows-latest, ubuntu-latest]
node-version: [14]
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-pnpm

yarn:
name: yarn package manager on ${{ matrix.node-version }} ${{ matrix.os }}
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
os: [windows-latest, ubuntu-latest]
node-version: [14]
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: |
yarn set version berry && yarn set version 2
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: [windows-latest, ubuntu-latest]
node-version: [14]
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: |
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
15 changes: 13 additions & 2 deletions lib/worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,19 @@ const build = require('pino-abstract-transport')

module.exports = async function ({ targets }) {
targets = await Promise.all(targets.map(async (t) => {
const toLoad = 'file://' + t.target
const stream = await (await import(toLoad)).default(t.options)
let fn
try {
const toLoad = 'file://' + t.target
fn = (await import(toLoad)).default
} catch (error) {
// See this PR for details: https://github.com/pinojs/thread-stream/pull/34
if ((error.code === 'ENOTDIR' || error.code === 'ERR_MODULE_NOT_FOUND')) {
fn = require(t.target)
} else {
throw error
}
}
const stream = await fn(t.options)
return {
level: t.level,
stream
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,8 @@
"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-pnpm": "pnpm run lint && tap --no-coverage --no-check-coverage test/*test.js test/*/*test.js && pnpm run test-types",
"test-ci-yarn-pnp": "yarn run lint && tap --no-check-coverage test/*test.js test/*/*test.js --coverage-report=lcovonly",
"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 @@ -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 }
44 changes: 25 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,26 @@ const writer = require('flush-write-stream')
const { pid } = process
const hostname = os.hostname()

async function installTransportModule () {
if (isYarnPnp) {
return
}
try {
await uninstallTransportModule()
} catch {}
await symlink(
join(__dirname, 'fixtures', 'transport'),
join(__dirname, '..', 'node_modules', 'transport')
)
}

async function uninstallTransportModule () {
if (isYarnPnp) {
return
}
await unlink(join(__dirname, '..', 'node_modules', 'transport'))
}

test('pino.transport with file', async ({ same, teardown }) => {
const destination = join(
os.tmpdir(),
Expand Down Expand Up @@ -53,21 +73,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 Down Expand Up @@ -297,14 +310,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 +319,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