jquery / jquery

jQuery JavaScript Library
https://jquery.com
MIT License
58.96k stars 20.62k forks source link

`require( "jquery" )` returns a module object when used with Webpack & jQuery 4.0.0-beta #5416

Closed mgol closed 2 months ago

mgol commented 3 months ago

The fact that we started exporting more than a single thing from jQuery - apart from the default export we have $ & jQuery named tokens pointing to the same thing - providing an ESM version of jQuery via the module field in package.json’s exports makes Webpack treat require( "jquery" ) as a way to export the full module instead of just the jQuery object...

Originally posted by @mgol in https://github.com/jquery/jquery/issues/5414#issuecomment-1937378557

Issue originally reported by @cocco111 in https://github.com/jquery/jquery/issues/5414

mgol commented 3 months ago

From @cocco111 in https://github.com/jquery/jquery/pull/5414#issuecomment-1937344478, we have a test case:

https://github.com/cocco111/jQuery4Test If you prefer you can simply open /dist/index.html in a browser and see result in console.

mgol commented 3 months ago

@GeoffreyBooth considering you were a strong advocate of us starting to expose named exports in addition to the default one - do you perhaps know how to fix this issue without stopping to expose those named tokens? Making require( "jquery" ) not return jQuery but instead a transpiled ESM module object when bundled with Webpack is going to break way too much code...

GeoffreyBooth commented 3 months ago

CommonJS doesn’t have a way to represent the “default plus named exports” feature of ES modules. The __esModule pattern that Babel and TypeScript use is meant to provide a workaround, for tools that understand the hint to treat the CommonJS export differently. You should probably ask a Webpack maintainer, though, as they would be the experts to advise how to output for maximum compatibility with their tool.

mgol commented 3 months ago

@sokra @vankop I'd appreciate some Webpack expertise from you here to help with setting up jQuery. We need the following to still work in jQuery 4.x, whether standalone or when bundled with Webpack or others:

const jQuery = require( "jquery" );
jQuery( "<div>" ).appendTo( "body" );

and:

import jQuery from "jquery";
jQuery( "<div>" ).appendTo( "body" );

At the same time, we cannot just serve two different versions of jQuery - one to require, one to import as there's lots of internal state that needs to be shared. Is this possible at all to achieve?

If I understand correctly, if we have Webpack pick up the module field from package.json's exports pointing to an ESM version of jQuery, require( "jquery" ) will always return a module object instead of jQuery which is the default export[^1]. The only way to avoid it seems to rely on separate import & require conditions in exports. To avoid data duplication issues, in Node.js we already serve a tiny wrapper over CommonJS in the import condition: https://github.com/jquery/jquery/blob/bf11739f6c6926bc9bc1b5a1460505d3b7ef8b01/package.json#L9-L13 https://github.com/jquery/jquery/blob/bf11739f6c6926bc9bc1b5a1460505d3b7ef8b01/dist-module/jquery.node-module-wrapper.js#L1-L4 but this assumes the ability to import from CommonJS files in ESM - which is true in Node.js but not necessarily in other tooling reading from exports. What we'd really need is a condition saying import-but-you-can-also-import-from-commonjs-here but there's no such thing. Some bundlers, like Rollup, only set the default, module & import conditions by default, for example.

[^1]: this is independent of whether we just expose the default export or more, I was wrong above

vankop commented 3 months ago

cc @alexander-akait

mgol commented 3 months ago

We discussed it today at the meeting. It seems for Webpack it may be enough to drop the module condition. That condition is already specified inside of the node one so any tool going there should support Node.js import/require semantics, in particular being able to import from a CommonJS module in an ESM one. Webpack supports that as expected.

mgol commented 3 months ago

That said, I think we have an issue with Rollup. Per https://www.npmjs.com/package/@rollup/plugin-node-resolve:

Additional conditions of the package.json exports field to match when resolving modules. By default, this plugin looks for the ['default', 'module', 'import'] conditions when resolving imports.

When using @rollup/plugin-commonjs v16 or higher, this plugin will use the ['default', 'module', 'require'] conditions when resolving require statements.

The node condition is not supported by default. Also, Rollup doesn't support require by default unless the @rollup/plugin-commonjs plugin is included.

That poses a problem - if the CommonJS plugin is loaded, we cannot just point import ... from "jquery" to a standalone ESM jQuery and require( "jquery" ) to a standalone CommonJS jQuery as that would result in two jQuery instances. jQuery has internal state so we need a single instance. A common way to resolve it is to avoid exposing a pure ESM jQuery and instead use a tiny wrapper over the CommonJS version, like we do for Node.js:

import jQuery from "../dist/jquery.js"; // "../dist/jquery.js" is a CommonJS file
const $ = jQuery;
export { jQuery, $ };
export default jQuery;

All good but then in a Rollup setup the @rollup/plugin-commonjs plugin may not be enabled and then the above would crash with something like:

[!] RollupError: "default" is not exported by "dist/jquery.js", imported by "src/app.js".

If there was a condition like import-but-this-tool-also-supports-require then we could differentiate between Rollup with the @rollup/plugin-commonjs plugin enabled or not - but we don't have anything like that.

@lukastaegert would you be able to help here or point us to someone who could help with making jQuery work with Rollup?

mgol commented 3 months ago

I'm surprised this is so hard to get right. At its core, the situation is quite simple and looks common.

We have a package a authored in CommonJS exposing a single API via module.exports. The library has internal state.

Bundlers enter the game and, despite the library not exposing anything ESM-related, they allow to do:

import a from "a";

in addition to the standard:

const a = require( "a" );

Later, authors of a decide to migrate it to ESM. There doesn't seem to be an easy way to achieve that in a way that preserves both above use cases when used with bundlers while not duplicating the library state.

The crux of the issue is that bundlers internally convert module.exports = a to export default a but if you expose export default a directly in a package, the CommonJS version exposed by Webpack becomes module.exports = { default: a }.

mgol commented 3 months ago

I've checked some scenarios with Rollup and Rollup seems to be doing something interesting. As long as the default export is a function, require( "a" ) is a wrapper function:

var a = function a () {
    if (this instanceof a) {
        return Reflect.construct(f, arguments, this.constructor);
    }
    return f.apply(this, arguments);
};

and calling it defers to the original function. However, extra APIs attached to that function are not proxied. In the jQuery case, this means the following:

const $ = require( "jquery" );
$.$( "<div>" );      // a jQuery object
$.jQuery( "<div>" ); // a jQuery object
$( "<div>" );        // a jQuery object <-- this works!
$.$.noop             // a noop function
$.jQuery.noop        // a noop function
$.noop               // undefined <-- this doesn't work!

Note that the ESM version: import $ from "jquery" - doesn't have $.$ or $.jQuery.

mgol commented 3 months ago

I tested Parcel with its experimental exports support enabled (https://parceljs.org/features/dependency-resolution/#enabling-package-exports) and there's no magic behavior for require( "a" ) like in Rollup, it just returns {__esModule: true, default: a}. However, it also doesn't enable the node condition by default as Webpack seems to do.

In practical terms, it's not much different from Rollup for us since we have utils attached directly to jQuery so Rollup's require magic is not sufficient for us.

mgol commented 3 months ago

cc @devongovett for the Parcel case.

GeoffreyBooth commented 3 months ago

Something worth considering is whether it might actually be okay to ship different builds to different targets for bundlers. For Node it’s an issue to ship a different instance for require as for import when each instance contains internal state, as within the same app some code might require the library while other code imports it; but is that really the case for bundlers? In a test app that both requires and imports jQuery, when built via Rollup or Parcel or Webpack, are two different instances of jQuery supplied in the build output? If not, then perhaps these tools can all simply get the ESM version, as it’s the more robust one (since it has the syntax to separately define a default export and named exports) and they can make require of that version work.

mgol commented 3 months ago

@GeoffreyBooth that's the issue - bundlers support passing an ESM version to require but then require( "jquery" ) returns { default: jQuery, $: jQuery, jQuery: jQuery } instead of jQuery and that's a huge breaking change that we cannot accept. Rollup adds some magic on top of this object as I posted above but that only allows to call the return value of require( "jquery" ) as a jQuery function but not use utils attached directly to jQuery.

To avoid that, we'd need to separate the import & require versions. And then we're hitting the double state issues while not being able to work around it as we do for Node.js since CommonJS support in these tools is not always enabled.

GeoffreyBooth commented 3 months ago

To avoid that, we'd need to separate the import & require versions.

But that's my point: if you separate them, does it fix the issue for bundlers? I would assume those tools only bundle one version or the other, not both?

If so, then the question becomes how to unbreak Node. But there's a node condition you could use.

mgol commented 3 months ago

But that's my point: if you separate them, does it fix the issue for bundlers? I would assume those tools only bundle one version or the other, not both?

I haven't tested this but I don't see how they could only bundle a single version in that case. I'm talking about projects using both import & require to fetch jQuery which is pretty common. Since the require( "jquery" ) call will then use a different entry from exports than an import ... from "jquery" call, both will need to be bundled. Bundlers cannot know those two versions are almost identical.

EDIT: just in case, I tested this with Rollup now and, just like I expected, if we have separate require & import conditions, Rollup will bundle jQuery twice.

cocco111 commented 3 months ago

Yes I can confirm words of @mgol . .module. and normal versions of jquery are nether same file nor same content (builder add code), so the bundler cannot understand they are the same. Having 2 copies of jQuery is also a problem for plugins ecosystem: in the way they are builded, they add code to first or second jquery

GeoffreyBooth commented 3 months ago

bundlers support passing an ESM version to require but then require( "jquery" ) returns { default: jQuery, $: jQuery, jQuery: jQuery }

What if the CommonJS version that bundlers get is a wrapper file? Like module.exports = require('jquery').default || require('jquery').

mgol commented 3 months ago

@GeoffreyBooth are you proposing changes to the logic in bundlers or to jQuery? If the latter, could you elaborate your idea? I don’t see how wrapping a CommonJS file in another CommonJS one in the jQuery repository is going to help here.

timmywil commented 3 months ago

CommonJS version that bundlers get

I think the issue is it's hard (impossible?) to differentiate between bundlers and not bundlers. And even then, what if the user does const { jQuery } = require('jquery')

mgol commented 3 months ago

Even if there was a condition only true for bundlers, it still wouldn’t help. What we need instead is a way to detect environments that support both import & require. This condition would be true for Node, Webpack & Rollup with the CommonJS plugin and it’d be false for Rollup without the CommonJS plugin.

devongovett commented 3 months ago

Maybe what could work would be to make two versions:

  1. For node, make CommonJS the main version, and have a small wrapper that re-exports it as an ES module. This works because node allows importing CJS from ESM, but not the other way around.
import jQuery from './jquery.node.cjs';
export default jQuery;
  1. For bundlers, make the ESM version the main one, and have a small wrapper that re-exports it as a CJS module. Bundlers allow requiring an ES module from CJS unlike node, and you can use this to remove the interop default object.
module.exports = require('./jquery.browser.esm.js').default;

But maybe I missed something.

mgol commented 3 months ago

@devongovett thanks for your input; what you wrote seems to work in Webpack, Rollup & Parcel! This may also be what @GeoffreyBooth meant by his last message that I misunderstood. We'd essentially reverse the flow for non-Node environments, assuming universal ESM support & non-universal CommonJS one there.

There's no condition true for all bundlers and only then but I'd consider applying Devon's suggestion for everything outside of the node section. The only question is - is there any important environment supporting CommonJS that is not Node.js and not a bundler? And that doesn't allow synchronously requiring an ESM file? If not, then we could just do:

  "exports": {
    ".": {
      "node": {
        "import": "./dist-module/jquery.node-module-wrapper.js",
        "require": "./dist/jquery.js"
      },
      "script": "./dist/jquery.min.js",
      "require": "./dist/jquery.bundler-require-wrapper.js",
      "default": "./dist-module/jquery.module.js"
    },
    ...
  }

where dist/jquery.bundler-require-wrapper.js is:

"use strict";

const jQueryModule = require( "../dist-module/jquery.module.js" );
module.exports = jQueryModule.default || jQueryModule;

The fallback to the full module may not even be needed, this is mostly to play safe in case a tool chooses a different way to transpile the default ESM export. Maybe it's not necessary.

mgol commented 3 months ago

Side-note: I removed the production & development conditions from my above proposal. They are reported by Webpack & Parcel but not by Rollup. The way we added them initially doesn't seem correct to me now, they should rather be additional conditions to consider for each existing condition. For example, my above proposal would look like the following with production & development considered:

  "exports": {
    ".": {
      "node": {
        "import": {
          "production": "./dist-module/jquery.node-module-wrapper.min.js",
          "default": "./dist-module/jquery.node-module-wrapper.js"
        },
        "require": {
          "production": "./dist/jquery.min.js",
          "default": "./dist/jquery.js"
        }
      },
      "script": "./dist/jquery.min.js",
      "require": {
        "production": "./dist/jquery.bundler-require-wrapper.min.js",
        "default": "./dist/jquery.bundler-require-wrapper.js"
      },
      "default": {
        "production": "./dist-module/jquery.module.min.js",
        "default": "./dist-module/jquery.module.js"
      }
    },
    ...
  }

That's pretty verbose and I don't think we really need it. This is perhaps more useful when a library has logic differences between development & production builds, other than just minifying the output.

mgol commented 2 months ago

The fix will be available in jQuery 4.0.0-beta.2 when that gets released.