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

reorganization to make it possible to run in the browser #80

Merged
merged 3 commits into from
Feb 26, 2025

Conversation

miguelgrinberg
Copy link
Contributor

@miguelgrinberg miguelgrinberg commented Feb 25, 2025

This change removes or makes optional the use of Node-specific elements, so that the library can also be used in a browser context.

More specifically:

  • Templates are pre-compiled to a .js file at build time, so that they do not need to be loaded from files at runtime.
  • Exception to the above, during tests the templates are still read from disk, so that Jest's watch option can catch updates.
  • The SubprocessExporter emits an error if it is used in the browser, because it needs exec(), which is only available in Node.
  • The list of HTTP methods is hardcoded, instead of importing it from the http module.

@miguelgrinberg miguelgrinberg force-pushed the no-node branch 5 times, most recently from b11956c to db943b3 Compare February 25, 2025 19:41
Copy link
Member

@JoshMock JoshMock left a comment

Choose a reason for hiding this comment

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

A few nits but LGTM!

Comment on lines +4 to +6
const sourceDir = (process.platform !== "win32")
? "src/exporters"
: "src\\exporters";
Copy link
Member

Choose a reason for hiding this comment

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

nit: you can use path.join to avoid having to do this detection yourself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I would need a polyfill for path for the browser. So far I managed to get everything working with only a polyfill for url, but I should probably try to eliminate that one as well.

miguelgrinberg and others added 2 commits February 26, 2025 15:49
Co-authored-by: Josh Mock <joshua.mock@elastic.co>
Co-authored-by: Josh Mock <joshua.mock@elastic.co>
@miguelgrinberg miguelgrinberg merged commit e5bcac4 into main Feb 26, 2025
23 checks passed
@miguelgrinberg miguelgrinberg deleted the no-node branch February 26, 2025 15:57
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.

2 participants