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

Jest tests display wrong line numbers when pem library (version 1.14.7 or 1.14.8) is added to project #389

Open
josephmfaulkner opened this issue Jan 25, 2024 · 5 comments

Comments

@josephmfaulkner
Copy link

It looks like the pem library is causing Jest tests to report the wrong line numbers for failed tests.

Initially, Jest displays the correct line that causes test to fail for a simple test.
SCREENSHOT-correct-line-reported

When pem is added to the project (uncomment out the first line on testedComponent.js), jest now reports the same failed test on the wrong line number.
SCREENSHOT-wrong-line-reported

This issue seems to have been present in the project since version 1.14.7
It doesn't reproduce for version 1.14.6 and below.

I've created a simple repository that reproduces this issue: https://github.com/josephmfaulkner/reproduce-jest-line-num-issue-when-using-pem

@nwalters512
Copy link

I think you've independently discovered the same issue I spent all afternoon tracking down. Could you try something to see if this is indeed the same issue? In node_modules/pem/dist/index.js, change require('./sourcemap-register.js'); to /* require('./sourcemap-register.js'); */ on the first line and let me know if you still see the wrong line numbers?

@nwalters512
Copy link

It seems like this can be resolved by importing from pem/lib/pem instead of just pem, e.g.:

import * as pem from 'pem/lib/pem';

@Dexus can you confirm if the lib directory is meant to be part of the public API? If so, I'll see about adding types for those to @types/pem so it's usable from TypeScript files, and possibly submitting a PR to this project documenting that in the README.

Can you share more about why the "main" field in package.json points to dist/index.js (which it looks like is bundled with ncc) as opposed to the unbundled files in lib? It seems like unbundled files are better all around, as they're possible to view/modify in node_modules and they avoid problems with sourcemaps like this issue describes.

@Dexus
Copy link
Owner

Dexus commented Apr 9, 2024

@nwalters512 pem/lib/pem is only a alternate to the bundled one. The different is that the pem bundeled does not push out all functions that the lib makes public. When I remember correctly.

@nwalters512
Copy link

@Dexus thanks for the reply. By "alternate", you mean that it's safe to rely on importing from lib/ directly, I hope? Now I'm curious about why the bundled version exists in the first place. From what I can tell, that was introduced in #311, but I don't see any discussion about the motivation.

@danfuzz
Copy link

danfuzz commented May 21, 2024

Hi! Just came here to note that pem's installing of its own sourcemap handler is problematic beyond just messing with error reports. See the linked bug above.

Beyond that, I don't actually want pem overriding globals in my system when not under test. It seems like there's a decent workaround, but maybe the fact that it does this by default should be called out in the docs. As with @nwalters512 I ended up spending quite a bit of frustrating time trying to figure out what was going wrong in my project.

danfuzz added a commit to danfuzz/lactoserv that referenced this issue May 21, 2024
danfuzz added a commit to danfuzz/lactoserv that referenced this issue May 21, 2024
Turns out `pem`'s main export will install its own global `process.emit`
source map handler, for who-the-muffin-knows-why. But in any case, (a)
it shouldn't, but thankfully (b) it's at least possible to avoid it
doing that. So this PR avoids the installation and adds a test to make
sure it doesn't start happening again. FWIW I was a bit wary when I
started using `pem` in the first place, and this experience makes me
reluctant to leave it in, in the long term.

See <Dexus/pem#389> and
<jestjs/jest#15077> for more details. And
thanks very much to @SimenB and @nwalters512 for their assistance.
danfuzz added a commit to danfuzz/lactoserv that referenced this issue May 21, 2024
This PR is a compendium of miscellanea, following up on the last few
days' worth of work.

Notably, this PR removes our use of the `pem` module from NPM as a
dependency, replacing it with `selfsigned`. TBQH the state of `pem` is
consistent with being in the early stage of a "supply chain" attack. See
<jestjs/jest#15077> and
<Dexus/pem#389> for some history / details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants