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

source-map-generator needs to be bundled #223

Closed
Tracked by #221
hildjj opened this issue Nov 30, 2021 · 4 comments
Closed
Tracked by #221

source-map-generator needs to be bundled #223

hildjj opened this issue Nov 30, 2021 · 4 comments
Labels
blocks-release Blocks an imminent release. High Priorty.

Comments

@hildjj
Copy link
Contributor

hildjj commented Nov 30, 2021

  1. Create a new directory.
  2. npm init
  3. npm i github:peggyjs/peggy#1.3
  4. npx peggy -h
Error: Cannot find module 'source-map-generator'
Require stack:
- <dir>/node_modules/peggy/lib/compiler/passes/generate-js.js
- <dir>/node_modules/peggy/lib/compiler/index.js
- <dir>/node_modules/peggy/lib/peg.js
- <dir>/node_modules/peggy/bin/peggy.js

I think it's already bundled into browser/peggy.min.js. It's not enough to use rollup to get it into the binary, since it will also be needed by the API.

@hildjj hildjj added the blocks-release Blocks an imminent release. High Priorty. label Nov 30, 2021
@hildjj hildjj mentioned this issue Nov 30, 2021
15 tasks
@hildjj
Copy link
Contributor Author

hildjj commented May 21, 2022

This will be the first full runtime dependency, so I want to make sure:
a) Nobody minds that
b) We make sure there are not entries in the docs that say "no runtime dependencies" still
c) We wouldn't rather bundle the dependency, like we do for commander - I don't think it would be as clean as the previous bundling without having the published library be all one .js file.
d) We think about unbundling commander and making it a runtime dependency.

@hildjj hildjj mentioned this issue May 23, 2022
@physikerwelt
Copy link

With runtime dependencies, do you mean runtime dependencies in the browser or a nodeJS dependency?
For my (test)case, it was sufficient to add the package as a dev dependency
https://gerrit.wikimedia.org/r/c/mediawiki/services/texvcjs/+/793836/2/package.json#38
and from looking at the diff of the generated source, it does not look as if there were new dependencies added.

@hildjj
Copy link
Contributor Author

hildjj commented May 26, 2022

I think that for the common browser case, we will continue to bundle dependencies with rollup. You'll still be able to do a single-line include and get things working. For the Node case, we will unbundle commander from the CLI, and add it and source-map-generator as runtime dependencies. For people that want to do their own bundling, as long as their tool does tree-shaking (to eliminate the unneeded commander dependency), they should get what they want from the node version.

We could also mark commander as optional, but I think node doesn't install optional dependencies by default anymore, so I think more people will be confused by that than leaving it as a full runtime dependency.

Does that make sense?

@hildjj
Copy link
Contributor Author

hildjj commented May 28, 2022

Fixed in v2.0.0

@hildjj hildjj closed this as completed May 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocks-release Blocks an imminent release. High Priorty.
Projects
None yet
Development

No branches or pull requests

2 participants