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

Issues when importing some functions from NcRichText.js #3864

Closed
julien-nc opened this issue Mar 6, 2023 · 12 comments
Closed

Issues when importing some functions from NcRichText.js #3864

julien-nc opened this issue Mar 6, 2023 · 12 comments
Labels
bug Something isn't working feature: richtext Related to the richtext component

Comments

@julien-nc
Copy link
Contributor

When importing registerWidget, registerCustomPickerElement or NcCustomPickerRenderResult from @nextcloud/vue/dist/Components/NcRichText.js the script fails and we get Uncaught TypeError: inspect is undefined in the browser console:

image

But this error goes away as soon as we import something from @nextcloud/vue-richtext, even an empty import like import {} from '@nextcloud/vue-richtext'.

This issue cannot be reproduced when linking with @nextcloud/vue (same with master or v7.8.0, it works fine).

So it seems there are some missing dependencies in the @nextcloud/vue package. Or can it be related with node-polyfill? I'm lost there.

To reproduce this bug, one can use this integration_tmdb branch: https://github.com/julien-nc/integration_tmdb/tree/enh/noid/replace-vue-richtext-by-vue , compile and enable the app in NC 26 or master and browse Talk or the Files app (with Text enabled).
The problematic import is in https://github.com/julien-nc/integration_tmdb/blob/enh/noid/replace-vue-richtext-by-vue/src/reference.js#L23-L24

cc @raimund-schluessler @juliushaertl

@julien-nc julien-nc added bug Something isn't working feature: richtext Related to the richtext component labels Mar 6, 2023
@julien-nc julien-nc added this to the 7.9.0 milestone Mar 6, 2023
@raimund-schluessler
Copy link
Contributor

I went through the (dev)dependencies list of vue-richtext again, and found that these packages

"mdast-util-from-markdown": "^1.2.0",
"mdast-util-gfm-autolink-literal": "^1.0.2",
"mdast-util-to-string": "^3.1.0",
"micromark-extension-gfm-autolink-literal": "^1.0.3",
"remark-disable-tokenizers": "^1.1.0",

are not present in nextcloud/vue anymore. I didn't install them in #3841 because I thought that they were unused (I didn't find where they were imported/used).
Maybe it helps adding them?

It would probably help to know which package includes inspect. Would it work to compare the bundles produced with and w/o the import from vue-richtext?

@mejo-
Copy link
Contributor

mejo- commented Mar 8, 2023

@julien-nc I'm unable to reproduce this issue with either your branch of integration_tmdb and with collectives. I did the following:

  • removed @nextcloud/vue-richtext from dependencies (completely gone from package-lock.json)
  • upgraded to latest @nextcloud/vue
  • used import { registerWidget } from '@nextcloud/vue/dist/Components/NcRichText.js'
  • compiled the app
  • tested using the smart picker and select elements found by the widget provided by collectives/integration_tmdb app

Anything I'm missing? 🤔

@julien-nc
Copy link
Contributor Author

I've tried it with server stable26 and master in Talk and Text: not working.
I'm using npm v8.15.0 and node v16.17.0.
Weird indeed. What could be different between our contexts?

@juliushaertl
Copy link
Contributor

Just sharing the screenshot I took when checking this a bit with @julien-nc

It was util -> console-browserify -> assert trying to call this

Screenshot 2023-03-06 at 21 50 26

My first thought was that https://www.npmjs.com/package/node-polyfill-webpack-plugin was missing but adding didn't make any difference.

@juliushaertl
Copy link
Contributor

Also worth to mention that this doesn't happen in server with nextcloud/server#33755

@raimund-schluessler raimund-schluessler modified the milestones: 7.9.0, 7.8.1 Mar 11, 2023
@julien-nc julien-nc modified the milestones: 7.8.1, 7.8.2 Mar 16, 2023
@mejo- mejo- modified the milestones: 7.8.2, 7.8.3 Mar 22, 2023
@nickvergessen nickvergessen modified the milestones: 7.8.3, 7.8.4, 7.8.5, 7.8.6 Mar 23, 2023
@juliushaertl
Copy link
Contributor

I could reproduce this locally with using npm pack instead of npm link, however have not really figured out what the difference is and why this doesn't work without the separate import.

Failing upstream lines in the trace
https://github.com/browserify/commonjs-assert/blob/master/internal/assert/assertion_error.js#L6
https://github.com/browserify/commonjs-assert/blob/master/internal/assert/assertion_error.js#L456

Steps to use npm pack

  • in @nextcloud/vue: npm ci && npm run dev && npm pack
  • Take the path to the freshly created tarball and add it to your apps package.json: "@nextcloud/vue": "file:~/repos/nextcloud/@nextcloud/nextcloud-vue/nextcloud-vue-7.9.0.tgz",
  • In your app run npm install && npm run dev

@julien-nc julien-nc modified the milestones: 7.8.6, 7.9.1 Apr 7, 2023
@julien-nc
Copy link
Contributor Author

@ShGKme Hey, do you have any clue about that? Could it be caused by the webpack config in nextcloud/vue and how the package is built?

@ShGKme
Copy link
Contributor

ShGKme commented Apr 14, 2023

@ShGKme Hey, do you have any clue about that? Could it be caused by the webpack config in nextcloud/vue and how the package is built?

better late than never

however have not really figured out what the difference is

Short answer:

  1. npm pack + update package.json + npm ci = Installing the library
  2. npm link = Link to a folder with the library

The difference between "The library" and "The folder with the library" is the dependency resolving:

  1. With npm pack you have a library and then dependencies are resolved by npm ci (or npm install):
    • NPM installs only lib's "production" dependencies to your app node_modules and resolves version conflicts
  2. With npm link in the linked library's folder you most likely already have node_modules folder:
    • For the library dependencies, this folder has a higher priority than your app's node_modules
    • This node_modules most likely also has devDependencies (because you built the lib earlier)
# With npm pack + npm ci
app
  node_modules/
    ...<app and app libs' production required dependencies>
    ...<production required @nextcloud/vue dependencies>
    @nextcloud/vue/
      node_modules/
        ...<production required @nextcloud/vue dependencies with version conflicts>

# With npm link
app
  node_modules/
    ...<app and app libs' production required dependencies>
    [linked] @nextcloud/vue
      node_modules/
        ...<all @nextcloud/vue dependencies>
        ...<all @nextcloud/vue devDependencies>

So, if some lib is a dependency of your app and your app's other libs and also a devDependency of the lib.

Right now I cannot say what exactly caused the problem. But I'm sure for 75%, that the problem is around wrong dependencies and devDependencies. The understanding of devDependencies for frontend libs is often not correct. It is not "development-tools deps". It is "not required to be installed with the lib dependencies". Or about "what is bundled with the lib".

I will look into the reproduction repo.

@ShGKme
Copy link
Contributor

ShGKme commented Apr 15, 2023

Sooo. I could reproduce it and found the problem after some funny hours.

Preface

  1. Some libs are isomorph (for example, axios) and uses native Node modules (such as util, process)
  2. In @nextcloud/webpack-vue-config we polyfill all native "Node" modules with node-polyfill-webpack-plugin
  3. All means ALL. Including, for example, console

Why it fails

  1. NcRichText depends on util native Node module polyfill
  2. util polyfill depends on console polyfill (link)
  3. console polyfill depends on util and assert native node modules
  4. 🎉 Congratulations: you have circular dependencies util -> console -> util 🔁
  5. But the circular dependencies is not the problem. While module is in process of executing it already exists as an (empty) object. Dependencies can save the reference to this object.
  6. assert polyfill depends on util (more circular dependencies)
  7. assert also REQUIRES util polyfill execution to be completed before. Because it uses not use a reference to the whole module object but takes a property from the module during evaluation
// 📁 browserify/commonjs-assert/assert.js
const { inspect } = require('util')
// While `util` is in process of evaluating, util = {}, inspect === undefined
// Later `util` will be evaluated. But `inspect` will remain undefined.

Why it works

It is enough to evaluate console before util 🏆

  1. @nextcloud/vue-richtext requires @nextcloud/axois -> axios -> buffer polyfill -> console polyfill
  2. console -> util -> console. On this moment util is successfully polyfilled (having a reference to an empty console polyfill module).
  3. console -> assert -> util now works, because util is already complete and cached 🥳

How to solve the problem

Console should not be polyfilling. To archive that we need to exclude it from node-polyfill-webpack-plugin:

// 📁 webpack.config.js
const webpackConfig = {
  plugins: [
    // ... other plugins ...
-   new NodePolyfillPlugin()
+   new NodePolyfillPlugin({
+     excludeAliases: ['console']
+   })
    // ... other plugins ... 
  ]
}

Workaround for integration_tmdb:

// 📁 webpack.js
// eslint-disable-next-line n/no-extraneous-require
const NodePolyfillPlugin = require('node-polyfill-webpack-plugin')
webpackConfig.plugins[1] = new NodePolyfillPlugin({
	excludeAliases: ['console'],
})

Note

Buffer is also available in the browser and should not be polyfelt.

@julien-nc
Copy link
Contributor Author

@ShGKme Thank you so much for the effort! You 🎸.
So in you opinion there is nothing to adjust in @nextcloud/vue but rather in apps that use it (via a fix in @nextcloud/webpack-vue-config), right?
In other words: Could this be fixed by adjusting something in @nextcloud/vue or do you think fixing @nextcloud/webpack-vue-config is the only way to go?

@ShGKme
Copy link
Contributor

ShGKme commented Apr 17, 2023

In other words: Could this be fixed by adjusting something in @nextcloud/vue or do you think fixing @nextcloud/webpack-vue-config is the only way to go?

It is not related to @nextcloud/vue because it is a problem with application bundling. It can be fixed either in a certain app or in @nextcloud/webpack-vue-config

@ShGKme
Copy link
Contributor

ShGKme commented Apr 18, 2023

I'm closing the issue as it is not @nextcloud/vue issue

@ShGKme ShGKme closed this as not planned Won't fix, can't repro, duplicate, stale Apr 18, 2023
@AndyScherzinger AndyScherzinger modified the milestones: 7.9.1, 7.10.0 Apr 18, 2023
@ShGKme ShGKme removed this from the 7.10.0 milestone Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature: richtext Related to the richtext component
Projects
None yet
Development

No branches or pull requests

7 participants