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

add bundler tests #4744

Open
trentm opened this issue May 31, 2024 · 5 comments
Open

add bundler tests #4744

trentm opened this issue May 31, 2024 · 5 comments

Comments

@trentm
Copy link
Contributor

trentm commented May 31, 2024

Issues come up frequently that are related to bundler usage -- at least with the node-related packages from this and the contrib repo. I think it would be helpful to have tests using recent versions of some bundlers (say: webpack, rollup, esbuild).

These tests would:

  • sanity check that we don't break something that currently works,
  • perhaps effectively define what bundler usage is actually supported (e.g. require-in-the-middle isn't going to work in a bundler, at least not without some bundler plugin -- like the current open issue to add esbuild support), and
  • provide a starting point for maintainers that may not be intimately familiar with the various bundlers to provide support on reported issues

I'm not sure exactly where these tests would live. Perhaps in @opentelemetry/instrumentation? Perhaps in sdk-node?

@AbhiPrasad
Copy link
Member

I think this should live in https://github.com/open-telemetry/opentelemetry-js/tree/main/integration-tests.

When testing bundling, I think it's also important to fully validate export conditions and built assets. In the Sentry repo we've had a lot of success with using Verdaccio to simulate a fake NPM registry, and then running an e2e test that downloads packages from the fake registry. Testing bundling on the fully built assets that show up in NPM (relying on package.json export conditions) is more robust in my eyes.

@pichlermarc
Copy link
Member

perhaps effectively define what bundler usage is actually supported (e.g. require-in-the-middle isn't going to work in a bundler, at least not without some bundler plugin -- like the current open issue to add esbuild support), and

Agree. I think this can be a separate issue and something we should document in @opentelemetry/instrumentation. At the moment, what we actually support is effectively only webpack for use with web instrumentations. As you said, bundled Node.js apps won't be instrumented without some bundler plugin, so I think calling that out in the README.md would make sense.

provide a starting point for maintainers that may not be intimately familiar with the various bundlers to provide support on reported issues

I think this would provide a lot of benefit. I often struggle with that as I don't use them in my daily life. It's often assumed that the bundler use is trivial but there's often many options for a bundler + typescript configs that can vary a lot and lead to different outcomes. Having a base that we can use to narrow stuff down and for people to provide reproducers would be extremely helpful.


I think this should live in https://github.com/open-telemetry/opentelemetry-js/tree/main/integration-tests.

+1 for putting it into ./integration-tests

When testing bundling, I think it's also important to fully validate export conditions and built assets. In the Sentry repo we've had a lot of success with using Verdaccio to simulate a fake NPM registry, and then running an e2e test that downloads packages from the fake registry. Testing bundling on the fully built assets that show up in NPM (relying on package.json export conditions) is more robust in my eyes.

Yes, I agree. This would be a nice-to-have. We've had numerous problems in the past as some package was

@AbhiPrasad
Copy link
Member

AbhiPrasad commented Jun 4, 2024

sweet - I'll take a look at this end of my week (thurs/friday) and open a POC PR to gather thoughts.

if anyone else wants to experiment before then though please feel free! You can ping me on slack if you have questions about this stuff, I consider myself slightly experienced with bundling 😄

Copy link

github-actions bot commented Aug 5, 2024

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Aug 5, 2024
@trentm trentm removed the stale label Aug 9, 2024
@trentm
Copy link
Contributor Author

trentm commented Aug 9, 2024

@AbhiPrasad Did you ever get a chance to start looking into bundler tests? No worries if not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants