-
Notifications
You must be signed in to change notification settings - Fork 421
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
exports is not defined on master #4818
Comments
with 7.12.10 it works fine, so the problem is caused by changes specifically in 7.12.11 |
PR here: #4819 |
as mentioned in the PR, the change will break with IE 11 so not possible to use it here. as suggested in #4819 (comment) the next part is to:
|
but first, there are other issues already on master with IE 11: #4820 |
nextcloud/notifications#825 (review) the problem is fixed by removing the |
okay, so should we move the ticket to backlog / 22 so we just discard the workaround at that time ? we already added the "upgrade exclusion" to avoid further regressions. |
this is a diff of the generated talk.js, on the left with preset-env 7.12.10 and on the right with 7.12.11:
|
ok, so the reason for |
I found some possible clues: it seems to be related to typescript / commonjs modules: https://stackoverflow.com/questions/54670544/how-to-fix-referenceerror-exports-is-not-defined-in-a-pure-typescript-project (the error we had was on @nextcloud/event-bus which is using typescript and compiling to commonjs) |
As far as I understand so far, webpack/babel needs some specific way of loading CommonJS for the browser. And why did it work so far ? Probably by pure luck, because something else was defining "exports" before, so this missing thing did not catch our eyes. |
Interesting, if I comment out the module here:
loading Talk now works fine... so maybe that exclusion was preventing babel to add whatever "exports" as necessary. Now that babel thing, as far as I understand, is mostly there to help with IE 11... But so far IE 11 was broken anyway, see #4820. So will need to continue debugging there as well for a more complete solution. |
instead of commenting that out, it seems that adding the commonjs plugin thing to that ".js" pattern block also solves the issue: diff --git a/webpack.common.js b/webpack.common.js
index 8fd837498..d195f2325 100644
--- a/webpack.common.js
+++ b/webpack.common.js
@@ -62,6 +62,17 @@ module.exports = {
'@nextcloud/event-bus',
'webdav',
]),
+ options: {
+ plugins: ['add-module-exports'],
+ presets: [
+ /**
+ * From "add-module-exports" documentation:
+ * "webpack doesn't perform commonjs transformation for
+ * codesplitting. Need to set commonjs conversion."
+ */
+ ['@babel/env', { modules: 'commonjs' }],
+ ],
+ },
},
{
/** |
also a mysterious thing is that the "server" did not need this part to work correctly even though it's using the event bus already in core, with the requesttoken. |
hmm, the server has an older version of vue, so maybe it could be the reason... |
the fix is included in #4829, closing |
Steps to reproduce
make dev-setup && make build-js
Expected behaviour
Web UI loads
Actual behaviour
Notes
Caused by #4811
We have no way to prevent dependabot to trigger this patch update.
Doesn't make sense that a patch update breaks compilation so suddenly ?
There are some ideas that it might be related to the "modules" babel option which isn't set in talk.
Also note that other repos like nextcloud-event-bus also haven't had that update yet, so there's a risk that breakage will happen there as well soon.
The text was updated successfully, but these errors were encountered: