Make optional jQuery require bundler-friendly (#4184)#4304
Open
BigBalli wants to merge 1 commit intojashkenas:masterfrom
Open
Make optional jQuery require bundler-friendly (#4184)#4304BigBalli wants to merge 1 commit intojashkenas:masterfrom
BigBalli wants to merge 1 commit intojashkenas:masterfrom
Conversation
The literal `require('jquery')` form (even inside try/catch) is
statically analyzed by Webpack, Rollup, esbuild and packd, so users
who do not actually want jQuery still see their bundles fail with
"Cannot find module 'jquery'".
Indirect the optional require through a variable that bundlers do
not follow:
var nodeRequire = typeof require === 'function' && require;
try { if (nodeRequire) $ = nodeRequire('jquery'); } catch (e) {}
Behavior is unchanged: jQuery remains optional, the try/catch still
swallows the runtime ENOENT in Node, and `Backbone.$` is still set
when jQuery is installed.
Also declare `jquery` as an optional peerDependency in package.json
so npm/yarn/pnpm document the relationship cleanly without forcing
the install.
Collaborator
|
Thank you for the awesome work in this pull request and the previous four! I made a superficial glance over all five PRs and they look very good to me. I will be reviewing them in more detail (and almost certainly also merging them) over the next two weeks or so. Do you expect any merge conflicts between them, and if so, which order would you recommend? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4184.
Problem
The CommonJS branch of the UMD wrapper does:
The
try/catchis correct at runtime in Node.js, but Webpack, Rollup,esbuild, packd and other bundlers statically analyze the literal
require('jquery')call before the try/catch ever runs. Users whodon't actually want jQuery still see their bundles fail with
Cannot find module 'jquery'.Fix
Indirect the optional require through a variable so the bundler's
static analysis does not follow it:
This is the same trick used by
wsand other libraries with optionalnative deps. Runtime behavior is unchanged: the require still happens
in Node, the try/catch still swallows ENOENT, and
Backbone.$isstill populated when jQuery is installed.
Also declare
jqueryas an optional peer dependency inpackage.jsonso npm/yarn/pnpm document the relationship cleanly without forcing the
install:
Test
npm run lintpasses.require('./backbone.js')in a fresh Node process withno jQuery installed: loads cleanly,
Backbone.VERSION === '1.6.1',Backbone.$ === undefinedas expected.