Closed cpiment closed 2 years ago
Great! I will review and test it!
If you need some help with the testing please let me know! I think that testing is harder than coding in this case and maybe I can help with that too so you are not by yourself.
Hi,
Well, I already checked the code, that is clean. I made several tests in dev mode, and it works pretty well! I didn't notice any bug.
But when I run yarn build
, I have a weird error, associated to 'discover' plugin (that is far away from enhanced-table plugin).
│ ERROR in C:/kibana-7.14/src/plugins/discover/public/application/apps/main/components/layout/discover_layout.scss?v7dark (C:/kibana-7.14/node_modules/css-loader/dist/cjs.js??ref--6-oneOf-0-1!C:/kibana-7.14/node_modules/postcss-loader/src??ref--6-oneOf-0-2!C:/kibana-7.14/node_modules/sass-loader/dist/cjs.js??ref--6-oneOf-0-3!C:/kibana-7.14/src/plugins/discover/public/application/apps/main/components/layout/discover_layout.scss?v7dark)
│ Module build failed (from C:/kibana-7.14/node_modules/sass-loader/dist/cjs.js):
│ SassError: File to import not found or unreadable: src/core/public/mixins.
│ on line 3 of C:\kibana-7.14\src\plugins\discover\public\application\apps\main\components\layout\discover_layout.scss
│ >> @import 'src/core/public/mixins';
│
│ ^
│
│ SassError: SassError: File to import not found or unreadable: src/core/public/mixins.
│ on line 3 of C:\kibana-7.14\src\plugins\discover\public\application\apps\main\components\layout\discover_layout.scss
│ >> @import 'src/core/public/mixins';
│
│ ^
But when I look to stack trace, I see that:
│ @ C:/kibana-7.14/src/plugins/visualizations/public/index.ts
│ @ ./public/enhanced-table-vis.js
│ @ ./public/plugin.ts
│ @ ./public/index.ts
│ @ C:/kibana-7.14/node_modules/@kbn/optimizer/target_node/worker/entry_point_creator.js
So the problems starts from enhanced-table.
I looked at public/enhanced-table-vis.js
, I clearly don't see any problem or dependency from an external SCSS file from kibana core. The only import is:
import { VIS_EVENT_TO_TRIGGER } from '../../../src/plugins/visualizations/public';
So I did a full compare between core "Data Table" plugin v7.12 and v7.14 (kibana/src/plugins/vis_type_table).
I see a first change in vis_controller.ts
(a try/catch block). This not tied to this build problem (I tested) but please can you bring this change, so that runtime errors are better managed?
Then can you tell me if you reproduce my issue when you launch "yarn build"?
Good news: using yarn compile
task, I can generate target folder.
Doing so, I can then generate package for myself, and install it on a standalone Kibana instance.
I did that and then did a full functional test coverage. Everything works great!
So it is finally not so annoying that yarn build
does not work.
I have had not time yet to test this, but I would like to try and fix the Kibana yarn build
and also implement the changes of vis_controller.ts
that you suggested. I will get into it ASAP.
Honestly, I'm quite pessimistic concerning yarn build
.
I don't think that the problem is caused by enhanced-table plugin.
I think that it is tied to kibana core.
can you push the commit on vis_controller.ts
?
I have also reproduced the yarn build
error in my environment. It's strange because the missing file src/core/public/mixins
is nowhere to be found anywhere in the Kibana folder.
The PR that changed the discover plugin is this: https://github.com/elastic/kibana/pull/96766 and it talks about removing angular from the discover plugin, so I think it is related to the build problem because there is a reference to an angular file in the stack trace of the error...
Maybe we can open a bug in Kibana, but since they are trying to get rid of all angular code I think they will not be able to help.
I also added the try/catch you requested.
Hi @cpiment,
Well, I completely understand that Kibana core aims to remove all its dependencies to AngularJS framework. But, when a plugin (whatever its under-hood framework) imports a simple class from Kibana core public API , it's not normal that it makes compilation fail (whereas it works fine in dev mode) because of a weird error in discover plugin (whereas enhanced-table plugin has no relation with discover plugin). So up to me, this is a Kibana core bug. That said, I hope that this issue has been fixed in a more recent Kibana version. And especially, I am reassured to know that there is a workaround that makes this issue, not a blocking issue.
Hi @cpiment,
Well, I completely understand that Kibana core aims to remove all its dependencies to AngularJS framework. But, when a plugin (whatever its under-hood framework) imports a simple class from Kibana core public API , it's not normal that it makes compilation fail (whereas it works fine in dev mode) because of a weird error in discover plugin (whereas enhanced-table plugin has no relation with discover plugin). So up to me, this is Kibana core bug. That said, I hope that this issue has been fixed in a more recent Kibana version. And especially, I am reassured to know that there is a workaround that makes this issue, not a blocking issue.
I have reproduced the issue importing the VIS_EVENT_TO_TRIGGER
into the kbn_tp_custom_visualization
plugin that is inside the test/plugin_functional/plugins
. Without the import I can call build
successfully but if I make that import it fails in the same way as your plugin, so as you said this seems to be something not related to your plugin. I'm going to upload the modified test plugin to a repository, check the behavior in 7.15 and 7.16 versions too and open a bug in Kibana repo. I will keep you posted.
Thanks for all the tests you did. it's reassuring to know that the issue is not because of enhanced-table plugin code.
Finally, thanks for your following tests and opening an issue on Kibana repository.
Thanks for this last change => LGTM!
Thanks for all the tests you did. it's reassuring to know that the issue is not because of enhanced-table plugin code.
Finally, thanks for your following tests and opening an issue on Kibana repository.
I have just filed the issue https://github.com/elastic/kibana/issues/121713.
While testing with my little test plugin I have confirmed that the error doesn't reproduce using Kibana 7.15.0 nor 7.15.1 source code, so maybe we can build the 7.14.2 plugin using the 7.15 source code, would that be possible?
Hi @cpiment,
Nice to see that you don't reproduce the issue with Kibana 7.15! By the way, if you don't reproduce the issue on Kibana 7.15, I'm afraid that your issue will never be processed. Elastic doesn't maintain past minor releases. They maintain only last minor version (here 7.16) and previous major version (here 6.8).
That said, I just released enhanced-table v1.12.1 with Kibana 7.14 support! https://github.com/fbaligand/kibana-enhanced-table/releases/tag/v1.12.1
I have a custom package script that first generate "target" folder (using yarn compile
) then build the package for every Kibana 7.14 versions. This has the advantage to be really fast compared to call yarn build
for every Kibana patch.
And so, that's why yarn build
broken is not a blocking issue, and why I don't need to use Kibana 7.15 source code.
I also made tests with Kibana 7.15 (standalone) to see if the same code is compatible, and sadly, there are breaking changes :( So a new migration work will be needed.
Opened PR #236 with the changes to make the plugin with 7.15.
Thanks for reviewing all this @fbaligand !!
Partially solves #197
Only needed changes in courier.ts file this time. I have made some tests and everything seems to be working...