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

Package manager CI action #1113

merged 22 commits into from
Sep 7, 2021

Conversation

Eomm
Copy link
Contributor

@Eomm Eomm commented Sep 1, 2021

Fix #1107
A run of this action here: https://github.com/Eomm/pino/actions/runs/1190604660

This PR requires a todo list to recap its status:

pnpm

  • no issues ✅ flaky test on win and node 16: disabled coverage to let it works

yarn

deep down on yarn@3 / PnP

The pino-elasticsearch issue is caused by the yarn's require.resolve() resolution that produces a string like this [...]cache/pino-elasticsearch-npm-6.1.0-0c03079478-6915435172.zip/node [...] (note the zip file).

This string is provided as fileName parameter to the thread-stream module:

https://github.com/pinojs/thread-stream/blob/main/lib/worker.js#L18

that throws the error:

+ENOTDIR: not a directory, stat '/Users/mspigolon/workspace/pino/.yarn/cache/pino-elasticsearch-npm-6.1.0-0c03079478-6915435172.zip/node_modules/pino-elasticsearch/lib.js'

Yarn supports this zip path out of the box (monkey patching it) but it does not support the import ESM loader.
Reference: yarnpkg/berry#638 and there is a draft PR to support it yarnpkg/berry#2161

So, to solve the PnP support issue, I see two main paths:

  1. update the thread-stream module to support yarn zip path using the @yarnpkg/fslib module.
  2. wait for the feat(pnp): experimental esm support yarnpkg/berry#2161 but I'm not sure it will work because the import is executed by the worker thread and I doubt it is monkey patched

@merceyz
Copy link

merceyz commented Sep 1, 2021

So, to solve the PnP support issue, I see two main paths:

A third option would be to fall back to require if import fails

but I'm not sure it will work because the import is executed by the worker thread and I doubt it is monkey patched

I haven't tested but it should work, the Worker is supposed to inherit the options from the parent

wait for the yarnpkg/berry#2161

That PR is mostly blocked by Node, they unflagged ESM before the loader API was ready

@mcollina
Copy link
Member

mcollina commented Sep 1, 2021

I like

A third option would be to fall back to require if import fails

a lot.

I'd prefer not to add that additional dependency, but I'm also ok to add it if it's the only option.

@merceyz
Copy link

merceyz commented Sep 1, 2021

I'd prefer not to add that additional dependency, but I'm also ok to add it if it's the only option.

Adding it wouldn't help much, you can read the file by using the fs module but import can't without yarnpkg/berry#2161

echo "nodeLinker: pnp" >> .yarnrc.yml
echo "pnpMode: loose" >> .yarnrc.yml
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

@Eomm
Copy link
Contributor Author

Eomm commented Sep 7, 2021

Quick update:

@mcollina
Copy link
Member

mcollina commented Sep 7, 2021

I'm ok in landing this without typescript support if it's feasible.

Comment on lines +54 to +55
# see https://github.com/yarnpkg/berry/issues/2935#issuecomment-911299992
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
# 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

.github/workflows/package-manager-ci.yml Outdated Show resolved Hide resolved
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

.github/workflows/package-manager-ci.yml Outdated Show resolved Hide resolved
@Eomm Eomm marked this pull request as ready for review September 7, 2021 13:35
@Eomm
Copy link
Contributor Author

Eomm commented Sep 7, 2021

PR ready

Updated first post accordingly.

for this issue

pnpm: there is one flaky test on Node v16 I'm working on

I had to disable the coverage since this test was starving.

What I found is that:

  • pnpm spawn tap
  • tap spawn a coverage instance and read the stdout
  • a tap test spawns a process
  • the process spawns a worker_thread
  • the tap test doesn't receive any close event

I did not investigate further

pino/test/transport.test.js

Lines 372 to 382 in f712a52

test('stdout in worker', async ({ not }) => {
let actual = ''
const child = execa(process.argv[0], [join(__dirname, 'fixtures', 'transport-main.js')])
child.stdout.pipe(writer((s, enc, cb) => {
actual += s
cb()
}))
await once(child, 'close')
not(strip(actual).match(/Hello/), null)
})

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

I think it might be too much to run this for every PR, those are a lot of tests. Maybe could we just run them.

As an example, we run this only on master: https://github.com/fastify/fastify/blob/main/.github/workflows/package-manager-ci.yml#L3-L6

I would also run this only on Node.js v14.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/transport.test.js Outdated Show resolved Hide resolved
test/transport.test.js Outdated Show resolved Hide resolved
test/transport.test.js Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit f462efa into pinojs:master Sep 7, 2021
@Eomm Eomm deleted the package-manager-load branch September 8, 2021 06:56
@github-actions
Copy link

github-actions bot commented Sep 9, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test pino.transport() with pnpm and yarn 2/3 pnp
3 participants