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

Problem with esrever dependency #3702

Closed
rudenate3 opened this issue May 27, 2020 · 12 comments
Closed

Problem with esrever dependency #3702

rudenate3 opened this issue May 27, 2020 · 12 comments

Comments

@rudenate3
Copy link

Do you want to request a feature or report a bug?

Bug

What's the current behavior?

Slate:
"slate": "^0.58.1",
"slate-history": "^0.58.1",
"slate-hyperscript": "^0.58.1",
"slate-react": "^0.58.1"

Browser: Chrome
OS: Mac / Windows / Linux

I am getting an error when trying to build my project:

Error: 'reverse' is not exported by node_modules/esrever/esrever.js, imported by node_modules/slate/dist/index.es.js

It is coming from this line:

import { reverse } from 'esrever';

I am able to fix the issue manually by changing this line to:

import esrever from 'esrever';
const { reverse } from esrever;

This seems functionally the same to me, but it works.

What's the expected behavior?

The dependency should provide the reverse function and build correctly.

Any help would be greatly appreciated 👍

@rudenate3
Copy link
Author

image

@cameracker
Copy link
Collaborator

cameracker commented May 28, 2020

I don't really know why your setup is failing here. The import, as far as I'm reading, is correct:
https://github.com/mathiasbynens/esrever/blob/master/src/esrever.js#L62

The way this is associated with the export should mean this is effectively a named export, so the import notation here is correct.

Additionally, the types are here: DefinitelyTyped/DefinitelyTyped#38048

Interested in whether other people are experiencing this issue. But currently I'm suspecting that this is related to some webpack or babel configuration you have, though I don't have any suggestions on where to look.

@TheSpyder
Copy link
Collaborator

TheSpyder commented May 28, 2020

I hit this too in rollup. I have to use namedExports config to avoid the error:

import commonjs from '@rollup/plugin-commonjs';
import nodeResolve from '@rollup/plugin-node-resolve';
import builtins from 'rollup-plugin-node-builtins';

  const plugins = [
    builtins(),
    nodeResolve({
      preferBuiltins: false,
      browser: true
    }),
    commonjs({
      namedExports: {
        [`${ROOTDIR}/node_modules/esrever/esrever.js`]: ['reverse'],
      }
    }),
    globals()
  ];

Note that I am also using the ES6 module of Slate, perhaps it's caused by esrever being commonjs only? I did this months ago and probably should have logged a ticket.

Clearly reverse is exported, rollup just can't find it when running from an ES6 module importing it.

@rudenate3
Copy link
Author

Sorry for the late reply, what @TheSpyder recommends is ultimately what fixed the issue! Thank you for the assistance!

@TheSpyder
Copy link
Collaborator

I think it would be preferable to not need the workaround, but it seems to be more of an esrever issue than anything Slate itself is doing. Unless Slate is willing to vendor it (the last commit to the project was over 2 years ago).

@TheSpyder
Copy link
Collaborator

The rollup commonjs plugin has fixed this issue, so namedExports is no longer required. Upgrading will show a warning.

@ScottAgirs
Copy link

namedExports: {
[${ROOTDIR}/node_modules/esrever/esrever.js]: ['reverse'],
}

Hey @TheSpyder , would ROOTDIR in this context be interchangeable with __dirname? What's the value of ROOTDIR ? 🙏🏼

@TheSpyder
Copy link
Collaborator

@ScottAgirs I don't think it was anything fancy, just const ROOTDIR = ".". We've switched from rollup to esbuild, and that's what we're using in the current script.

Since the bundle tool is always run through yarn bundle, and yarn always executes commands from the project root, it doesn't need to be complicated.

@ScottAgirs
Copy link

ScottAgirs commented May 9, 2021

@TheSpyder thank you for that! I thought so, so I tried ./node_m..., node_m...,  ${__dirname}/node_m..., but still keep getting the same error :/

I'm running on latest errr-ting.

Here's my package:

{
    ...
    "slate": "0.62.1",
    "slate-deep-table": "^0.9.7",
    "slate-react": "^0.62.1",
    "slate-soft-break": "^0.9.0",
    "theme-ui": "^0.8.4"
  },
  "devDependencies": {
    "@ijsto/eslint-config": "4.1.6",
    "@rollup/plugin-babel": "^5.3.0",
    "@rollup/plugin-commonjs": "^19.0.0",
    "@rollup/plugin-json": "^4.1.0",
    "@rollup/plugin-node-resolve": "^13.0.0",
    "eslint": "^7.26.0",
    "rollup": "^2.47.0",
    "rollup-plugin-eslint": "^7.0.0"
  }

and my rollup:

export default {
  input: 'src/index.js',
  output: {
    dir: 'dist',
    format: 'cjs',
  },
  plugins: [
    babel({
      babelHelpers: 'bundled',
      exclude: 'node_modules/**',
      presets: ['@babel/preset-env', '@babel/preset-react'],
    }),
    commonjs({
      namedExports: {
        [`${__dirname}/node_modules/esrever/esrever.js`]: ['reverse'],
      },
    }),
    nodeResolve(),
    json(),
  ],
};

Does that look right-ish? 🙈

@TheSpyder
Copy link
Collaborator

This is probably the wrong place to ask - as I said I don't use rollup anymore - please try Slack.

Also note I did say

The rollup commonjs plugin has fixed this issue, so namedExports is no longer required. Upgrading will show a warning.

@ScottAgirs
Copy link

This is probably the wrong place to ask - as I said I don't use rollup anymore - please try Slack.

Also note I did say

The rollup commonjs plugin has fixed this issue, so namedExports is no longer required. Upgrading will show a warning.

@TheSpyder awesome, thanks! Briefly skimming through and trying esbuild makes me think this would not be a trivial switch, would it?

Fair enough, thanks, will try in slatejs slack

@TheSpyder
Copy link
Collaborator

TheSpyder commented May 9, 2021

Briefly skimming through and trying esbuild makes me think this would not be a trivial switch, would it?

It depends what you're doing. It was easy for my team, but the project wasn't really leveraging advanced rollup capabilities (no React, and we output ES6 so we don't need babel at all).

The insane performance difference was worth any headaches - we bundle in about 150 milliseconds.

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

No branches or pull requests

4 participants