Import does not work anymore since version 4.4.0

Question

Import does not work anymore since version 4.4.0

gizmodus opened this issue 5 years ago · 20 comments

We use xregexp in a shared library that is used in our client (Angular 10) as well as on our server (NodeJS / Webpack 5). Until version 4.3.0 we were able to import xregexp like this and everything worked as expected:

const XRegExp = require('xregexp');

Now we wanted to upgrade to version 4.4.0, but get the following error in the client application (Angular 10):

TypeError: XRegExp.matchRecursive is not a function

The server still works as expected.

Changing the import statement to const XRegExp = require('xregexp').default; solves the problem in the client but leads to this error on the server: TypeError: Cannot read property 'matchRecursive' of undefined.

We also tried import * as XRegExp from 'xregexp'; but this does not work either. Any ideas why this suddenly does not work anymore?

Answer 1 · 2020-11-08T23:36:48.000Z

Facing the same issue.

Answer 2 · 2020-11-08T23:44:23.000Z

This could be a side effect of having upgraded most of the dependencies in v4.4.0, or it could have stemmed from adding TypeScript definitions in the same release. Any pull requests that fix this are most welcome.

Answer 3 · 2020-11-09T19:17:49.000Z

I suspect this may be an Angular issue, see #294

Answer 4 · 2020-11-18T10:38:52.000Z

This bug was introduced by the pull request #281 with this commit 99ee554 by adding "module": "./src/index.js"

So lets see why this is causing issues.
Bundlers like webpack which support CommonJs and ESModules will start using ESModules from xregexp 4.4.0 instead of the previous CommonJs integration from 4.3.0.

require('xregexp') // --> function (commonJs)

After the change we get an object instead which includes the function as a default property:

require('xregexp') // -> { esModule: true, default: function } (esModules)

@slevithan can we just revert #281 ?

Answer 5 · 2020-11-18T14:25:13.000Z

If you're using webpack and not using esm, you should probably configure webpack to ignore module main fields since you are going against the grain.

https://webpack.js.org/configuration/resolve/#resolvemainfields

Answer 6 · 2020-11-18T14:29:24.000Z

For the OP, use:

import XRegExp from 'xregexp';

Answer 7 · 2020-11-18T14:32:21.000Z

@jsg2021 we are using a lot of es6 node_modules for better tree shaking so disabling the module field isn't an option for us

right now we are using the @adobe/htlengine which is using xregexp somewhere in its xss validaiton logic: https://github.com/adobe/htlengine/blob/ffcfeecc1d60e0f8f47c15be68ce794a8620b6d6/src/runtime/xss_api.js#L17

your change broke our entire pipeline :)

if you say all xregexp webpack users need to change their webpack config and or switch from require to import might be a possible way however that's a breaking change

Answer 8 · 2020-11-18T14:44:58.000Z

My PR is more than 9mo old and I think XRegExp was still on 3.x...

To your point, the evidence says this change is worthy of a major version bump.

Answer 9 · 2020-11-18T15:05:13.000Z

4.4.0 was released 20 days ago and this issue was filed 14 days ago

xregexp 4.3.0 has no module field:
https://unpkg.com/xregexp@4.3.0/package.json

xregexp 4.4.0 has a module field:
https://unpkg.com/xregexp@4.4.0/package.json

I don't know why it got into the 4.4.0 release..

So there are two options:

we find a way to fix the issue for 4.5.0
we revert the change for 4.5.0 and release the changes from 4.4.0 as 5.0.0

However option 2 would mean that xregexp can't be used anymore in any isomorphic environment

Answer 10 · 2020-11-18T15:20:58.000Z

However option 2 would mean that xregexp can't be used anymore in any isomorphic environment

Why would it mean that? front-end and back-end code could use it just the same. Just older commonjs code would have to adapt.

Answer 11 · 2020-11-18T15:32:38.000Z

For node 12 there is only commonJs - that's why the @adobe/htlengine and other isomorphic packages use commonJs

However if require('xregexp') works differently for browser and node environments this approach will not work anymore

Answer 12 · 2020-11-18T15:41:32.000Z

For node 12 there is only commonJs - that's why the @adobe/htlengine and other isomorphic packages use commonJs

However if require('xregexp') works differently for browser and node environments this approach will not work anymore

That's fair, but 14 is the current LTS, so that would just be a small window of awkward transition.

Another option to try in the meantime, is to add xregexp to the externals so that webpack doesn't bundle it and lets node process the require().

Answer 13 · 2020-11-18T19:40:47.000Z

I think I'm okay with requiring node 14 in an XRegExp 5 (any issues can perhaps be noted in the "Installation and usage" section of README.md), but I'm not actively supporting XRegExp these days and I'm not very familiar with the details of cjs/umd/esm, rollup, babel, webpack, etc. so these kinds of changes are harder for me to test.

I'd be happy to merge and release pull requests with all changes needed for an XRegExp v4.5.0 revert and v5.0.0 reintroduction of this change (possibly adding @jsg2021's #282 for v5). If anyone wants to take that on, I'd really appreciate it, and would additionally appreciate the descriptions being clear and including release notes (like https://github.com/slevithan/xregexp/releases) that I can attach when publishing. (No need to do version bumping like c3342b5 unless you want to since I can handle that.)

Answer 14 · 2020-11-18T20:04:32.000Z

Cool thanks @slevithan 👍

for 4.5 all we need to do is to delete the module entry in package.json.. I can prepare a pr for this

for 5.0 I have an idea how we can make use of the changes @jsg2021 did also for node 12:

commonJs

   module.exports = { default: XRegExp }

es modules (just like before)

  export default XRegExp

that way the typings would work correctly and it would be isomorphic as both ways would work e.g.:

import XRegExp from 'XRegExp';
require('XRegExp').default

another possible solution which might be even cleaner would be

commonJs

   module.exports = { XRegExp }

es modules

  export const XRegExp

the imports would look like:

import {XRegExp} from 'XRegExp';
require('XRegExp').XRegExp

Answer 15 · 2020-11-19T18:49:47.000Z

Thanks, @jantimon. Regarding the two options shown, I'd prefer to make sure the following still works:

import XRegExp from 'xregexp';

Answer 16 · 2020-11-20T08:51:55.000Z

Okay personally I don't like to allow renaming libraries as it often leads to different naming in projects:

import XRegExp from 'xregexp';
import xRegexp from 'xregexp';
import xregexp from 'xregexp';

But its totally valid anyway and if you prefer that way I'll prepare a pull request :)
The only downside is that in commonJs the import would look like this require('XRegExp').default

Answer 17 · 2020-12-08T03:25:35.000Z

Hey all, maintainer who merged #281 here. I'm still wrapping my head around this issue, but I tend to agree with the following comments in that this is a webpack bug and/or configuration issue:

#305 (comment)

If you're using webpack and not using esm, you should probably configure webpack to ignore module main fields since you are going against the grain.

webpack.js.org/configuration/resolve/#resolvemainfields

#305 (comment)

Another option to try in the meantime, is to add xregexp to the externals so that webpack doesn't bundle it and lets node process the require().

Here's why: I installed Node 14 (to get module support), and ran both import and require versions of the code, getting the same result from each (see the Details block below). If webpack is supposed to make it so that you can use the module in the browser, the same way that you can from the server, then it doesn't seem to be working properly here, and needs to be configured differently, as @jsg2021 suggested in their comments above.

Let me know if I've overlooked anything. I do realize that even if webpack is technically wrong here, it might be easier to just work around it, rather than requiring XRegExp users to change their code, so I'm certainly still open to that.

$ node --version
v14.13.1
$ cat test.mjs
───────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: test.mjs
───────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ import XRegExp from 'xregexp';
   2   │ console.log(XRegExp)
   3   │ console.log(XRegExp())
───────┴──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
$ node test.mjs
[Function: XRegExp] {
  version: '4.4.0',
  _clipDuplicates: [Function: clipDuplicates],
  _hasNativeFlag: [Function: hasNativeFlag],
  _dec: [Function: dec],
  _hex: [Function: hex],
  _pad4: [Function: pad4],
  addToken: [Function (anonymous)],
  cache: [Function (anonymous)] { flush: [Function (anonymous)] },
  escape: [Function (anonymous)],
  exec: [Function (anonymous)],
  forEach: [Function (anonymous)],
  globalize: [Function (anonymous)],
  install: [Function (anonymous)],
  isInstalled: [Function (anonymous)],
  isRegExp: [Function (anonymous)],
  match: [Function (anonymous)],
  matchChain: [Function (anonymous)],
  replace: [Function (anonymous)],
  replaceEach: [Function (anonymous)],
  split: [Function (anonymous)],
  test: [Function (anonymous)],
  uninstall: [Function (anonymous)],
  union: [Function (anonymous)],
  tag: [Function (anonymous)],
  build: [Function (anonymous)],
  matchRecursive: [Function (anonymous)],
  addUnicodeData: [Function (anonymous)],
  _getUnicodeProperty: [Function (anonymous)]
}
/(?:)/ { xregexp: { captureNames: null, source: '', flags: '' } }
$ cat test.js
───────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
       │ File: test.js
───────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1   │ const XRegExp = require('xregexp');
   2   │ console.log(XRegExp)
   3   │ console.log(XRegExp())
───────┴──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
$ node !$
node test.js
[Function: XRegExp] {
  version: '4.4.0',
  _clipDuplicates: [Function: clipDuplicates],
  _hasNativeFlag: [Function: hasNativeFlag],
  _dec: [Function: dec],
  _hex: [Function: hex],
  _pad4: [Function: pad4],
  addToken: [Function (anonymous)],
  cache: [Function (anonymous)] { flush: [Function (anonymous)] },
  escape: [Function (anonymous)],
  exec: [Function (anonymous)],
  forEach: [Function (anonymous)],
  globalize: [Function (anonymous)],
  install: [Function (anonymous)],
  isInstalled: [Function (anonymous)],
  isRegExp: [Function (anonymous)],
  match: [Function (anonymous)],
  matchChain: [Function (anonymous)],
  replace: [Function (anonymous)],
  replaceEach: [Function (anonymous)],
  split: [Function (anonymous)],
  test: [Function (anonymous)],
  uninstall: [Function (anonymous)],
  union: [Function (anonymous)],
  tag: [Function (anonymous)],
  build: [Function (anonymous)],
  matchRecursive: [Function (anonymous)],
  addUnicodeData: [Function (anonymous)],
  _getUnicodeProperty: [Function (anonymous)]
}
/(?:)/ { xregexp: { captureNames: null, source: '', flags: '' } }
$

Answer 18 · 2020-12-08T03:39:27.000Z

Actually, after reading https://webpack.js.org/configuration/resolve/#resolvemainfields more closely, it sounds like adding a browser field to package.json, that points to lib/index.js (same as main) should fix things.

resolve.mainFields
[string]

When importing from an npm package, e.g. import * as D3 from 'd3', this option will determine which fields in its package.json are checked. The default values will vary based upon the target specified in your webpack configuration.

When the target property is set to webworker, web, or left unspecified:

webpack.config.js

module.exports = {
//...
resolve: {
mainFields: ['browser', 'module', 'main']
}
};
For any other target (including node):

webpack.config.js

module.exports = {
//...
resolve: {
mainFields: ['module', 'main']
}
};
For example, consider an arbitrary library called upstream with a package.json that contains the following fields:

{
"browser": "build/upstream.js",
"module": "index"
}
When we import * as Upstream from 'upstream' this will actually resolve to the file in the browser property. The browser property takes precedence because it's the first item in mainFields. Meanwhile, a Node.js application bundled by webpack will first try to resolve using the file in the module field.

I'll open a PR for that.

Answer 19 · 2020-12-08T05:34:50.000Z

Alright, I just published v4.4.1 with the fix from #308, can y'all confirm it works for you?

Answer 20 · 2021-11-15T10:59:49.000Z

We also tried import * as XRegExp from 'xregexp'; but this does not work either. Any ideas why this suddenly does not work anymore?

After adding

resolve: {
  alias: {
    xregexp: path.resolve(
      __dirname,
      '..',
      'node_modules',
      'xregexp',
      'lib',
      'index.js'
    )
  }
}

to the webpack config, we are able to use

import * as XRegExp from 'xregexp';

with xregexp: "5.0.2".