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.

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

Import does not work anymore since version 4.4.0 #305

Closed
gizmodus opened this issue Nov 4, 2020 · 20 comments · Fixed by #308
Closed

Import does not work anymore since version 4.4.0 #305

gizmodus opened this issue Nov 4, 2020 · 20 comments · Fixed by #308

Comments

@gizmodus
Copy link

gizmodus commented Nov 4, 2020

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?

@sachithd
Copy link

sachithd commented Nov 8, 2020

Facing the same issue.

@slevithan
Copy link
Owner

slevithan commented Nov 8, 2020

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.

@josephfrazier
Copy link
Collaborator

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

@jantimon
Copy link

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 ?

@jsg2021
Copy link
Contributor

jsg2021 commented Nov 18, 2020

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

@jsg2021
Copy link
Contributor

jsg2021 commented Nov 18, 2020

For the OP, use:

import XRegExp from 'xregexp';

@jantimon
Copy link

jantimon commented Nov 18, 2020

@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

@jsg2021
Copy link
Contributor

jsg2021 commented Nov 18, 2020

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.

@jantimon
Copy link

jantimon commented Nov 18, 2020

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:

  1. we find a way to fix the issue for 4.5.0
  2. 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

@jsg2021
Copy link
Contributor

jsg2021 commented Nov 18, 2020

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.

@jantimon
Copy link

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

@jsg2021
Copy link
Contributor

jsg2021 commented Nov 18, 2020

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().

@slevithan
Copy link
Owner

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.)

@jantimon
Copy link

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

@slevithan
Copy link
Owner

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

import XRegExp from 'xregexp';

@jantimon
Copy link

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

@josephfrazier
Copy link
Collaborator

josephfrazier commented Dec 8, 2020

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: '' } }
$

@josephfrazier
Copy link
Collaborator

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.

josephfrazier added a commit to josephfrazier/xregexp that referenced this issue Dec 8, 2020
This should close slevithan#305, see slevithan#305 (comment):

> 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.
slevithan pushed a commit that referenced this issue Dec 8, 2020
This should close #305, see #305 (comment):

> 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.
@josephfrazier
Copy link
Collaborator

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

@dtslvr
Copy link

dtslvr commented Nov 15, 2021

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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants