tscanlin / tocbot

Build a table of contents from headings in an HTML document.
MIT License
1.39k stars 115 forks source link

Added ESM version breaks storybook build #340

Closed jklepek-vendavo closed 4 months ago

jklepek-vendavo commented 5 months ago

Hello, we are using Storybook in our library, which has tocbot as a dependency. After minor upgrade of Storybook, it no longer builds. Investigation revealed that after adding ESM version, storybook build in our library was broken: Error: No "exports" main defined in ./node_modules/tocbot/package.json Storybook version is 7.6.17, which has tocbot as a dependency at ^4.20.0.

Workaround for now is to override tocbot version.

TheDutchCoder commented 5 months ago

The whole ESM build fails. We're also using tocbot in a Nuxt 3 setting and the latest version fails with: The requested module '/_nuxt3/@fs/Users/reinierkaper/rvezy/rvezy-web-client/node_modules/.cache/vite/client/deps/tocbot.js?v=0644d02d' does not provide an export named 'default'

In addition: all the scss exports are also gone, so you can no longer use @import 'tocbot/src/scss/tocbot.scss';

tscanlin commented 5 months ago

Hey, sorry about this! Recently there was an update to support ESM modules and it must have caused issues. I pushed some fixes and it should address this issue. tocbot@4.27.3 has the fixes and should be good to go. Let me know if that helps solve this. Thanks! https://github.com/tscanlin/tocbot/commit/79cddddecb840a55a371e0c1a137a138be74d1ad https://github.com/tscanlin/tocbot/commit/2fca913ca936e3ba868b15df1f3e71c91f3c0e37

Josh-Walker-GM commented 5 months ago

Hey đź‘‹ I hope this is the right thread to continue in but I am continuing to see issues around the imports from this package with storybook even with v4.27.3:

image

I could be mistaken but it looks like https://github.com/tscanlin/tocbot/commit/79cddddecb840a55a371e0c1a137a138be74d1ad changes the export signature in a breaking way?

tscanlin commented 5 months ago

Hi @Josh-Walker-GM, I pushed a fix to add those exports back, can you try tocbot@4.27.4?

Josh-Walker-GM commented 5 months ago

Looks like that's fixed it for us. Thanks for the incredibly quick turn around!

zhangzhenxi0316 commented 5 months ago

image hi @tscanlin , I encountered this problem as shown above image i think u need set default import config in package.json like this image After I manually added default exports, the build went normally. The strange thing is that whether I export the esmodule product or the common js product, it can be built successfully in the end.

stefano-aizon commented 5 months ago

Getting a Storybook (8.0.4) error with tocbot@4.27.4

Failed to resolve entry for package "tocbot". The package may have incorrect main/module/exports specified in its package.json: No known conditions for "." specifier in "tocbot" package [plugin vite:dep-pre-bundle]
tscanlin commented 5 months ago

@zhangzhenxi0316 I'll try adding that. Thanks for the suggestion!

@stefano-aizon are you using commonjs or ESM to import it?

tscanlin commented 5 months ago

@zhangzhenxi0316 tocbot@4.27.5 is published with that change. Let me know if that helps. Thanks!

stefano-aizon commented 5 months ago

@tscanlin It's used by Storybook so I'm not too sure. But tocbot@4.27.5 solved the issue 🥳. Thank you!

jklepek-vendavo commented 5 months ago

Hello, I tried with the new version, but the build still fails:

./node_modules/tocbot/index.js:1 import { destroy, init, refresh } from './src/js/index-esm.js' ^^^^^^

SyntaxError: Cannot use import statement outside a module

zhangzhenxi0316 commented 5 months ago

@tscanlin image hi~,please check package.json config, The default parameter needs to be at the bottom of the export configuration thanks

danplubell commented 5 months ago

We are also experiencing this issue in Storybook: /Users/myproject/node_modules/.pnpm/tocbot@4.27.5/node_modules/tocbot/index.js:1 import { destroy, init, refresh } from './src/js/index-esm.js' ^^^^^^

tscanlin commented 5 months ago

Thanks @zhangzhenxi0316! I moved it to the bottom in this latest release 4.27.6. let me know if that resolves it for you.

@danplubell do you have a public repo somewhere where I can reproduce this issue? Or alternatively can you help me figure out what generates the TOC in storybook?

@jklepek-vendavo are you using storybook too? Or are you just trying to use via commonjs? If you need to use via commonjs can you try changing how its imported to be like this? const tocbot = require('tocbot/dist/tocbot.js')

danplubell commented 5 months ago

@tscanlin Here is an example that demonstrates the issue in storybook: https://stackblitz.com/edit/github-7s5dz5?file=.storybook%2Fmain.ts The @storybook/blocks is the storybook add-on in which we are seeing the issue.

tscanlin commented 5 months ago

Ok, this should be working again in the latest version tocbot@4.27.12.

Thanks for your help troubleshooting @danplubell! :)

danplubell commented 5 months ago

@tscanlin It looks like the latest version resolves our issue. Thanks for the resolution!

maxarias-io commented 5 months ago

With Bun, you can override until Storybook updates:

...
"overrides": {
    "tocbot": "4.27.13"
  }

Make sure to remove node_modules and reinstall

jklepek-vendavo commented 5 months ago

With Bun, you can override until Storybook updates:

...
"overrides": {
    "tocbot": "4.27.13"
  }

Make sure to remove node_modules and reinstall

There should be no need for this, as they have it defined as ^4.20. so it is upgraded to latest 4.2* version automatically (at least with npm)

jklepek-vendavo commented 5 months ago

I can confirm, that the latest version works for me too :) thanks for so fast fixes!

dchambers commented 5 months ago

Hi there đź‘‹, 4.27.13 is still failing for me (when running Jest tests configured to use ESM) owing to the fact that the default module type has been left as CJS, yet index.js imports src/js/index-esm.js which also needs to be processed an an ESM (rather than a CJS).

This can be fixed by adding "type": "module", to the package.json, and this will also work fine for the CJS entry-point (dist/tocbot.js) since it's a bundle, and so has no need to import other CJS files into itself.

tscanlin commented 5 months ago

@dchambers sorry to hear its still not working for you. How are you importing tocobt? Also, what is the full error that you are seeing? Can you create an example somewhere which shows the issue? Unfortunately its not quite as easy as adding "type": "module", to package.json as that seems to break the build. But I will try and look into it some more. Thanks for the heads up on this!

dchambers commented 5 months ago

Hi @tscanlin đź‘‹, thanks so much for you help with this issue BTW.

We don’t directly import from tocbot, but we instead have a component which imports from @storybook/blocks, and the error occurs when running the unit tests for that component in a version of Jest configured to use Jest’s ESM support:

This is the error message we see:

  â—Ź Test suite failed to run

    Jest encountered an unexpected token

    Jest failed to parse a file. This happens e.g. when your code or its dependencies use non-standard JavaScript syntax, or when Jest is not configured to support such syntax.

    Out of the box Jest supports Babel, which will be used to transform your files into valid JS based on your Babel configuration.

    By default "node_modules" folder is ignored by transformers.

    Here's what you can do:

     • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/ecmascript-modules for how to enable it.
     • If you are trying to use TypeScript, see https://jestjs.io/docs/getting-started#using-typescript
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/configuration
    For information about custom transformations, see:
    https://jestjs.io/docs/code-transformation

    Details:

    /Users/dominic.chambers/Development/form-renderer/node_modules/tocbot/index.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){import { destroy, init, refresh } from './src/js/index-esm.js'
                                                                                      ^^^^^^

    SyntaxError: Cannot use import statement outside a module
      at Runtime.createScriptFromCode (../../node_modules/jest-runtime/build/index.js:1505:14)

I've personally seen the 'Cannot use import statement outside a module' error quite a bit when using Node.js directly, and so I'd imagine you'd see this error if you were to create a Node.js project with "type": "module", and have it import the built tocbot/index.js file.

For context, we use Jest’s ESM support because it allows to verify that the hybrid ESM/CJS modules that we export will actually work when consumed from various ESM toolchains. We use quite a few tools in our toolchain, and one observation I’ve had is that different tools have different levels of strictness over which transgressions from the ESM standards that they’ll accept, where Jest’s ESM tooling (which is based on Node.js’s ESM support) seems to be one of the strictest.

tscanlin commented 5 months ago

@dchambers I have a PR up that adds type: module. Can you check it out and see if it solves the issue you are seeing? https://github.com/tscanlin/tocbot/pull/343/files#diff-8080bc761f4219a4012924d57634e10258c09e614f0906e0e2c9313ebc63e0d1R3

you should be able to install the commit from github: https://stackoverflow.com/questions/14187956/npm-install-from-git-in-a-specific-version git://github.com/tscanlin/tocbot.git#8080bc761f4219a4012924d57634e10258c09e614f0906e0e2c9313ebc63e0d1R3

Hopefully that solves the issue, but let me know if it doesn't.

dchambers commented 5 months ago

@tscanlin, thanks so much for the rapid response on this. I struggled a little grabbing from Git, and so this is the config I ended up using:

"overrides": {
    "tocbot": "tscanlin/tocbot#fix-esm-export"
}

where the test are successfully passing âś…

tscanlin commented 5 months ago

Awesome, glad to hear! I've merged the PR and kicked off a release. 4.27.17 has the type: 'module' change.

Thanks for your help validating this @dchambers! đź‘Ť