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.

Sign up for GitHub

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

Refactor Inquire so it can resolve modules in Browsers with default CPS #1548

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Refactor Inquire so it can resolve modules in Browsers with default CPS #1548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db43fc4
remove eval from inquire and update documentation
tinder-seanlang-brown Feb 5, 2021
0539d68
remove rebase artifact from pacakge.json
tinder-seanlang-brown Feb 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/inquire/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,10 @@ API
* **inquire(moduleName: `string`): `?Object`**<br />
Requires a module only if available.

Browser Bundler Compatibility
---
Inquire can be used in browser modules if these configurations are set:
1) If a bundler is used, all modules loaded with inquire() must be added to the bundle config field "externals". This is supported by [webpack](https://webpack.js.org/configuration/externals/), [browserify](https://github.com/browserify/browserify#usage) ([gulp](https://benclinkinbeard.com/posts/external-bundles-for-faster-browserify-builds/)), [rollup](https://rollupjs.org/guide/en/#inputoptions-object), and possibly others.
2) When used in node/npm packages for distribution (including protobufjs) all modules loaded with inquire() must be [ignored in the package.json browser option](https://github.com/defunctzombie/package-browser-field-spec#ignore-a-module). This is also [supported by webpack, browserify and rollup](https://github.com/webpack/webpack/issues/8826#issuecomment-491081733).

**License:** [BSD 3-Clause License](https://opensource.org/licenses/BSD-3-Clause)
13 changes: 12 additions & 1 deletion lib/inquire/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,24 @@ module.exports = inquire;

/**
* Requires a module only if available.
* All modules requested with inquire must be set as externals in browserify.json, e.g.
* "externals": [
* "long",
* "buffer"
* ]
* and added ignored in the package.json browser field, e.g.
* "browser": {
* "long": false,
* "buffer": false
* }
*
* @memberof util
* @param {string} moduleName Module to require
* @returns {?Object} Required module if available and not empty, otherwise `null`
*/
function inquire(moduleName) {
try {
var mod = eval("quire".replace(/^/,"re"))(moduleName); // eslint-disable-line no-eval
var mod = require(moduleName);
Copy link
Member

Choose a reason for hiding this comment

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

The original idea here was to have a way to require only if require is present (on node), in turn making it necessary that bundlers do not recognize this, yet still having protobuf.js as a dependency somewhere in a graph doesn't blow up. With this change, the common case seems to become that stuff blows up, unless every dependent goes through the hassle of changing their configs.

Copy link

Choose a reason for hiding this comment

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

@dcodeIO Yes, as you say, I have kept the original intended behavior by setting the externals in the package.json, which creates a trade-off between using eval() and having to set configs. If that's not acceptable (I understand - it adds maintenance effort), could we create a distribution of ProtobufJS intended for the browsers that does not use eval at all, called Protobufjs/minimal-browser? It seems if you are okay with inquire not working in most browsers, then there's no reason to include it.

This should be easy to configure with gulp: we can replace inquire with a dummy function that always returns null. As I see it, there's no point to calling eval() if it is blocked by the browser, and the message that the browsers print make people think that there is a problem. What do you think?

Copy link

Choose a reason for hiding this comment

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

@dcodeIO Following up on this one more time - I think no config changes are needed by dependent projects. The changes to package.json in this repo prevent dependents from blowing up.
I'm sorry I did not clearly address your concern in my last comment. If I can demonstrate that no dependent config changes are needed, would you reconsider this PR?

Copy link
Member

Choose a reason for hiding this comment

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

I am mostly out of the loop and it's hard for me to see all the implications. Mostly putting a comment here or there for other reviewers :)

if (mod && (mod.length || Object.keys(mod).length))
return mod;
} catch (e) {} // eslint-disable-line no-empty
Expand Down
4 changes: 2 additions & 2 deletions lib/inquire/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@protobufjs/inquire",
"description": "Requires a module only if available and hides the require call from bundlers.",
"description": "Requires a module only if available.",
"version": "1.1.0",
"author": "Daniel Wirtz <[email protected]>",
"repository": {
Expand All @@ -18,4 +18,4 @@
"test": "tape tests/*.js",
"coverage": "istanbul cover node_modules/tape/bin/tape tests/*.js"
}
}
}
4 changes: 4 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@
],
"main": "index.js",
"types": "index.d.ts",
"browser": {
"long": false,
"buffer": false
},
"scripts": {
"bench": "node bench",
"build": "npm run build:bundle && npm run build:types",
Expand Down
6 changes: 6 additions & 0 deletions scripts/browserify.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"externals": [
"long",
"buffer"
]
}
3 changes: 2 additions & 1 deletion scripts/bundle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ var vinylfs = require("vinyl-fs");
var source = require("vinyl-source-stream");

var pkg = require(path.join(__dirname, "..", "package.json"));
var { externals } = require("./browserify.json");

/*eslint-disable no-template-curly-in-string*/
var license = [
Expand Down Expand Up @@ -50,7 +51,7 @@ function bundle(options) {
prelude: prelude,
preludePath: "./lib/prelude.js"
})
.external("long");
.external(externals);
if (options.exclude)
options.exclude.forEach(bundler.exclude, bundler);
return bundler
Expand Down