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

chore: update import paths to use .js file extensions #89

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

zckrs
Copy link
Contributor

@zckrs zckrs commented Jul 23, 2024

To follow #32, #33 and fix build for ESM

@zckrs zckrs requested a review from a team as a code owner July 23, 2024 13:04
@CLAassistant
Copy link

CLAassistant commented Jul 23, 2024

CLA assistant check
All committers have signed the CLA.

@bryanhuhta
Copy link
Contributor

👋 Hey @zckrs! Thanks for this contribution. I just merged some lint changes (requiring semicolons) which created a lot of conflicts with this PR. I also ran CI on this PR and it looks like specifying .js in the imports is breaking many of the tests. I am investigating this right now.

@zckrs
Copy link
Contributor Author

zckrs commented Aug 20, 2024

Hi @bryanhuhta 👋

I just rebase on main and reapply diff

How can I help you to merge this ?

@bryanhuhta
Copy link
Contributor

@zckrs I appreciate your patience 🙇

I validated this patch works on both a ESM project and a CJS one, but the test suite does not appreciate these changes. When I run the tests, I get lots of

 FAIL  test/profiler.test.ts
  ● Test suite failed to run

    Cannot find module '../src/index.js' from 'test/profiler.test.ts'

    > 1 | import Pyroscope from '../src/index.js';
        | ^
      2 | import express from 'express';
      3 | import busboy from 'busboy';
      4 | import { Profile } from 'pprof-format';

      at Resolver.resolveModule (node_modules/jest-resolve/build/resolver.js:324:11)
      at Object.<anonymous> (test/profiler.test.ts:1:1)

Presumably because there is a mismatch between how jest is configured to resolve modules and how the modules are built. I've been trying to find a solution, but my free time is limited. The only blocker to this change is making sure the test suite can run with these changes in place.

@zckrs
Copy link
Contributor Author

zckrs commented Sep 4, 2024

In fact Jest doesn't have a good support for ESM :/

https://jestjs.io/docs/ecmascript-modules

@nwalters512
Copy link
Contributor

@bryanhuhta since it seems like Jest is the blocker here, would y'all welcome a PR to rewrite the tests with Mocha, Vitest, or some other test runner of your choice that better supports modern JS features like ESM?

@nwalters512
Copy link
Contributor

I went ahead and opened #94 to migrate to Vitest. Hopefully that can unblock proper ESM support.

@bryanhuhta
Copy link
Contributor

bryanhuhta commented Sep 11, 2024

@zckrs I just merged a few PRs which should unblock this PR (#94 is the real work horse here):

Can you merge origin/main and fix the merge conflicts? I'll approve and merge this PR after that, then cut a new release.

@nwalters512
Copy link
Contributor

nwalters512 commented Sep 11, 2024

I tested these changes locally on top of the latest main. Tests are passing, and the output in dist/ looks as it should! 🎉

Very excited to see this land. @bryanhuhta if @zckrs can't resolve the conflicts soon, you should be able to do so on his behalf and get this merged. If not, I'd (selfishly) suggest opening a new PR with conflicts resolved so that the next release isn't blocked.

@zckrs
Copy link
Contributor Author

zckrs commented Sep 11, 2024

Hi,
Need to sleep but after that I will rebase from main branch 😉

@bryanhuhta bryanhuhta requested a review from a team as a code owner September 11, 2024 21:16
@bryanhuhta bryanhuhta merged commit 02486d2 into grafana:main Sep 11, 2024
6 checks passed
@zckrs zckrs deleted the fix-esm-build branch September 11, 2024 21:29
@bryanhuhta
Copy link
Contributor

@zckrs @nwalters512 I merged this and released v0.4.0. Let me know if there's any problems that arise!

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

Successfully merging this pull request may close these issues.

4 participants