-
Notifications
You must be signed in to change notification settings - Fork 44
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 browser compatible builds #110
Conversation
a1ae927
to
6ff37b9
Compare
Thanks a lot for looking into this! I like what see here a lot 👏 For clarification: We're building a "special" browser-focused ES module here, so we're don't put it into the @jorgecasar: If you find the time, would you mind reviewing this, too? For one thing, you have been following the browser use case; and looked into #38 as well. |
333d2e0
to
988f4b9
Compare
Signed-off-by: Aron Carroll <self@aroncarroll.com>
Signed-off-by: Aron Carroll <self@aroncarroll.com>
Signed-off-by: Aron Carroll <self@aroncarroll.com>
Signed-off-by: Aron Carroll <self@aroncarroll.com>
See: https://nodejs.org/api/packages.html#conditional-exports Signed-off-by: Aron Carroll <self@aroncarroll.com>
Signed-off-by: Aron Carroll <self@aroncarroll.com>
77eb8f3
to
8ec1936
Compare
Signed-off-by: Aron Carroll <self@aroncarroll.com>
Signed-off-by: Aron Carroll <self@aroncarroll.com>
Signed-off-by: Aron Carroll <self@aroncarroll.com>
Signed-off-by: Aron Carroll <self@aroncarroll.com>
I've updated the description, hopefully this clarifies things a little. I ended up doing a bunch more reading on the different fields in the package.json file. It turns out the Node in general supports both esm and commonjs modules and will prefer the one used by the importing project. We now just use a shim called TypeScript I think will still use the CommonJS version. |
Great work @aron. For browser, I consider a good practice expose packages in ESM and delegate to the user to transpile to ES5 if needed. It could be great if we set the source code to ESM and build the commonJs version for older version of nodeJS. But it could be done in a close future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much! 💯
Using deno was enabled by open-policy-agent#110. Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Using deno was enabled by open-policy-agent#110. Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Using deno was enabled by open-policy-agent#110. Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Using deno was enabled by #110. Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Okay, so this PR adds a few new pieces that enable browser compatible builds as well as clearer entrypoints for the node modules too. The following table illustrates the compatibility matrix now provided.
These entrypoints are exposed in the package.json file using the
exports
property supported by Node 12 and newer. Older Node versions will fallback to themain
field and require the commonjs package.require()
to import the module.import opa from "..."
to import the module.opa
global (this could be updated to use a commonjs/umd shim by adding--banner
and--footer
properties as part of the build).I've added some examples to the bottom of the README to show how these would be used.
Node Modules
Not much has changed in the core code, I've added the two entrypoints
index.cjs
andindex.mjs
to support the two module types as described in the Node docs. I've also fixed the TextEncoding/Decoding that was discussed previously.Browser Modules
This change adds a new
dist
directory containing the two files described above. These are now generated by the./build.sh
script and use esbuild. I've found that pretty easy to use and very fast. I looked into microbundle as well but that project is not aimed at commonjs packages nor for bundling dependencies. Webpack and Rollup required too much configuration for me.Once published both of these generated modules should then be consumable from a CDN like unpkg for use directly in a webpage or projects can install and consume the package directly.
Testing
I've tested this locally using
npm link
to run it on a few test projects. Creating a node esm project and commonjs project both work fine. As does the commonjs project on Node 10.I've created a basic integration script that uses pupetteer to verify that the browser packages also work as expected. This doesn't do a whole lot just imports the global script and then the esm script and verifies that the policy can be loaded and evaluated.
TypeScript
I moved the TypeScript files into the
dist
directory too to keep things simple and I've moved the generation into the./build.sh
script. We still generate files for the all the modules though it looks like onlyopa.d.ts
is used, I've tested this locally with a TypeScript project and my editor picks up the types as expected. I've updated thetypes
field in the package.json.Package & Publishing
I've rejigged a bunch of the package.json, we now have the new
exports
field mentioned above and the updatedtypes
field. The last part was to add thefiles
property to define a safelist of files to be included in the bundle. The current output fromnpm pack
is below, I'd appreciate it if you could let me know if anything is missing.Output from
npm pack --json
:This change also opens up the door for #38 if you are still looking to convert the codebase over to the new module system. Though I would potentially wait until moving away from node 10 to do this otherwise you're going to need to produce a commonjs compatible build just for that version.
Let me know what you think so far, apologies for making it into a bigger change than potentially needed, feel free to take whichever bits are useful. I've got a little more time next week to look into this.
As for next steps, I think if this merges it would be a good idea to try and publish an alpha/beta version first just to ensure that everything is still working in the wild.