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.

Sign up for GitHub

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

Jump to bottom

[ZEPPELIN-5792][FRONT_BUILD] Remove bower #4651

Merged
merged 6 commits into from
Sep 25, 2023
Merged

[ZEPPELIN-5792][FRONT_BUILD] Remove bower #4651

merged 6 commits into from
Sep 25, 2023

Conversation

matthias-koch
Copy link
Contributor

What is this PR for?

Remove deprecated bower. As the "bower" says, recommended using a different system. So it looks good to remove bower. Replace bower by npm.

What type of PR is it?

Improvement

Todos

  • Bower's dependencies are resolved by npm
  • Minor dependency versions may have been changed to resolved by npm
  • Integrate new dependencies to front-end and testing

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-5792

How should this be tested?

  • Run npm commands
  • Run Karma tests
  • Manual front-end testing

Screenshots (if appropriate)

Questions:

  • Does the license files need to update? Maybe because of library version change
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

Hint

This is a resubmit of #4633

pan3793
pan3793 reviewed Sep 11, 2023
View reviewed changes
@Reamer
Copy link
Contributor

Reamer commented Sep 14, 2023

I have completed the JUnit5 migration today. Can you please rebase your branch to the current master branch.

@matthias-koch matthias-koch force-pushed the ZEPPELIN-5792_remove_bower branch from beb60cf to c8c0a2d Compare September 15, 2023 06:19
Reamer
Reamer previously approved these changes Sep 15, 2023
View reviewed changes
Copy link
Contributor

@Reamer Reamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for two small things, I think the PR is good. Unfortunately I'm not a NodeJS developer and can't give a deep review.

pan3793
pan3793 reviewed Sep 16, 2023
View reviewed changes
zeppelin-web/README.md Outdated
@@ -12,49 +12,49 @@ All build commands are described in [package.json](./package.json)

```sh
# install required depepdencies and bower packages (only once)
Copy link
Member

@pan3793 pan3793 Sep 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems there are few places still refer bower, could you please do a global search to find them out?

@matthias-koch matthias-koch force-pushed the ZEPPELIN-5792_remove_bower branch from c284d22 to 8a789c4 Compare September 19, 2023 12:01
Reamer
Reamer approved these changes Sep 22, 2023
View reviewed changes
Copy link
Contributor

@Reamer Reamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@jongyoul & @pan3793 What do you think?

pan3793
pan3793 approved these changes Sep 22, 2023
View reviewed changes
Copy link
Member

@pan3793 pan3793 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - based on my limited knowledge in frontend tech stack

@jongyoul
Copy link
Member

LGTM as well. BTW, it will help to reduce our compile time :-)

@jongyoul jongyoul merged commit ee3b053 into apache:master Sep 25, 2023
@Reamer Reamer mentioned this pull request Sep 25, 2023
Closed
3 tasks
@matthias-koch matthias-koch deleted the ZEPPELIN-5792_remove_bower branch September 25, 2023 13:35
akoira pushed a commit to akoira/zeppelin that referenced this pull request Feb 1, 2024
@matthias-koch @holodazoltan
[ZEPPELIN-5792][FRONT_BUILD] Remove bower (apache#4651)
dd75cb7 
* [ZEPPELIN-5792][FRONT_BUILD] Remove bower

Co-Authored-By: holodazoltan <holoda@gmail.com>

* [ZEPPELIN-5792] Fix broken package-lock.json

* ZEPPELIN-5792 Wait before checking welcome page

* [ZEPPELIN-5792] Remove npm update

* [ZEPPELIN-5792] remove npm update

* [ZEPPELIN-5792] Remove bower references

---------

Co-authored-by: holodazoltan <holoda@gmail.com>
@Armadik
Copy link
Contributor

Armadik commented May 30, 2024 •
edited
Loading

@jongyoul

I'm not good at web technologies, but it seems that this PR broke the web interface. I launch notepad, and after some time it becomes unavailable for changes, restarting the page helps, in the logs I only see:

`


vendor.e8bbdaad8068699d.js:51 Uncaught
ReferenceError: autoApply is not defined at n._onCloseHandler (vendor.e8bbdaad8068699d.js:51:134262) at WebSocket.<anonymous> (vendor.e8bbdaad8068699d.js:21:5609)
  | n._onCloseHandler | @ | vendor.e8bbdaad8068699d.js:51 -- | -- | -- | --   | (anonymous) | @ | vendor.e8bbdaad8068699d.js:21

`

However, I cannot reproduce the problem locally. It is reproduced only on the instance in k8s and ldap integration, I don’t understand how this can be connected.

@jongyoul
Copy link
Member

jongyoul commented Jun 6, 2024

@Armadik Hello, Thank you for the report. Could you please make a JIRA ticket for this issue? I think someone can help to investigate it. I also don't know why it's connected yet so let me check it as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Reamer Reamer Reamer approved these changes

@jongyoul jongyoul jongyoul approved these changes

@pan3793 pan3793 pan3793 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@matthias-koch @Reamer @jongyoul @Armadik @pan3793