You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@zeppelin.apache.org by "matthias-koch (via GitHub)" <gi...@apache.org> on 2023/09/11 09:51:57 UTC
[GitHub] [zeppelin] matthias-koch opened a new pull request, #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
matthias-koch opened a new pull request, #4651:
URL: https://github.com/apache/zeppelin/pull/4651
### 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
* [x] Bower's dependencies are resolved by npm
* [x] Minor dependency versions may have been changed to resolved by npm
* [x] 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 https://github.com/apache/zeppelin/pull/4633
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] pan3793 commented on a diff in pull request #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#discussion_r1327966977
##########
zeppelin-web/README.md:
##########
@@ -12,49 +12,49 @@ All build commands are described in [package.json](./package.json)
```sh
# install required depepdencies and bower packages (only once)
Review Comment:
seems there are fews place still refers `bower`, could you please do a global search to find them out?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on pull request #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#issuecomment-1719681560
I have completed the JUnit5 migration today. Can you please rebase your branch to the current master branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] pan3793 commented on a diff in pull request #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#discussion_r1321601463
##########
pom.xml:
##########
@@ -111,8 +111,8 @@
<scalacheck.version>1.12.5</scalacheck.version>
<!-- frontend maven plugin related versions-->
- <node.version>v12.3.1</node.version>
- <npm.version>6.9.0</npm.version>
+ <node.version>v14.17.0</node.version>
Review Comment:
why choose an EOL version?
https://endoflife.date/nodejs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] pan3793 commented on a diff in pull request #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#discussion_r1327966871
##########
zeppelin-web/README.md:
##########
@@ -12,49 +12,49 @@ All build commands are described in [package.json](./package.json)
```sh
# install required depepdencies and bower packages (only once)
-$ npm install -g yarn
-$ yarn install
+$ npm install -g npm
Review Comment:
I think the change is fine.
There are bunch of tools state they are better replacements of `npm`, including `yarn`, `pnpm` etc., but `npm` still be the default one shipped with NodeJS, I think only mention `npm` in guide is a better choice
##########
zeppelin-web/README.md:
##########
@@ -12,49 +12,49 @@ All build commands are described in [package.json](./package.json)
```sh
# install required depepdencies and bower packages (only once)
-$ npm install -g yarn
-$ yarn install
+$ npm install -g npm
Review Comment:
I think the change is fine.
There are bunch of tools state they are better replacements of `npm`, including `yarn`, `pnpm` etc., but `npm` still is the default one shipped with NodeJS, I think only mention `npm` in guide is a better choice
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-5792][FRONT_BUILD] Remove bower [zeppelin]
Posted by "jongyoul (via GitHub)" <gi...@apache.org>.
jongyoul commented on PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#issuecomment-2151289045
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] matthias-koch commented on a diff in pull request #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
Posted by "matthias-koch (via GitHub)" <gi...@apache.org>.
matthias-koch commented on code in PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#discussion_r1321406490
##########
zeppelin-web-angular/pom.xml:
##########
@@ -80,7 +80,7 @@
</goals>
<configuration>
<skip>${web.e2e.enabled}</skip>
- <arguments>install --no-lockfile</arguments>
+ <arguments>install --legacy-peer-deps</arguments>
Review Comment:
Because I have updated the npm version for performance reasons, this parameter must be specified so that the new npm version works like the old one. See also the answer to this question on Stack Overflow: [https://stackoverflow.com/questions/66239691/what-does-npm-install-legacy-peer-deps-do-exactly-when-is-it-recommended-whether](https://stackoverflow.com/questions/66239691/what-does-npm-install-legacy-peer-deps-do-exactly-when-is-it-recommended-whether)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] pan3793 commented on a diff in pull request #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#discussion_r1327966977
##########
zeppelin-web/README.md:
##########
@@ -12,49 +12,49 @@ All build commands are described in [package.json](./package.json)
```sh
# install required depepdencies and bower packages (only once)
Review Comment:
seems there are few places still refer `bower`, could you please do a global search to find them out?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
Posted by "jongyoul (via GitHub)" <gi...@apache.org>.
jongyoul commented on code in PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#discussion_r1328067422
##########
zeppelin-web/README.md:
##########
@@ -12,49 +12,49 @@ All build commands are described in [package.json](./package.json)
```sh
# install required depepdencies and bower packages (only once)
-$ npm install -g yarn
-$ yarn install
+$ npm install -g npm
Review Comment:
He means that If we can execute `npm install -g npm` itself, it means `npm` should already be executable. Thus, we don't need to execute `npm install -g npm` again.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] matthias-koch commented on a diff in pull request #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
Posted by "matthias-koch (via GitHub)" <gi...@apache.org>.
matthias-koch commented on code in PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#discussion_r1322513826
##########
pom.xml:
##########
@@ -111,8 +111,8 @@
<scalacheck.version>1.12.5</scalacheck.version>
<!-- frontend maven plugin related versions-->
- <node.version>v12.3.1</node.version>
- <npm.version>6.9.0</npm.version>
+ <node.version>v14.17.0</node.version>
Review Comment:
To avoid trouble, I updated to the oldest Node version that is still supported by npm 9. However, this should actually be done in a separate PR. I removed the npm update.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
Posted by "jongyoul (via GitHub)" <gi...@apache.org>.
jongyoul commented on code in PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#discussion_r1321616603
##########
pom.xml:
##########
@@ -111,8 +111,8 @@
<scalacheck.version>1.12.5</scalacheck.version>
<!-- frontend maven plugin related versions-->
- <node.version>v12.3.1</node.version>
- <npm.version>6.9.0</npm.version>
+ <node.version>v14.17.0</node.version>
Review Comment:
I'm not an expert but some newer versions of nodes cannot build the current Zeppelin's code as it is. I agree that we need to update to the latest version but we could have another PR if it leads to a lot of changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] pan3793 commented on a diff in pull request #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#discussion_r1321621130
##########
pom.xml:
##########
@@ -111,8 +111,8 @@
<scalacheck.version>1.12.5</scalacheck.version>
<!-- frontend maven plugin related versions-->
- <node.version>v12.3.1</node.version>
- <npm.version>6.9.0</npm.version>
+ <node.version>v14.17.0</node.version>
Review Comment:
If so, why not stay at node 12 to minimize the scope of this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a diff in pull request #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on code in PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#discussion_r1327245470
##########
zeppelin-web/README.md:
##########
@@ -12,49 +12,49 @@ All build commands are described in [package.json](./package.json)
```sh
# install required depepdencies and bower packages (only once)
-$ npm install -g yarn
-$ yarn install
+$ npm install -g npm
Review Comment:
Is this step necessary or is this a SearchAndReplace mistake?
##########
zeppelin-web/pom.xml:
##########
@@ -102,7 +102,7 @@
<executions>
<execution>
- <id>install node</id>
+ <id>install node and npm-install</id>
Review Comment:
```suggestion
<id>install node and npm</id>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] Reamer commented on a diff in pull request #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
Posted by "Reamer (via GitHub)" <gi...@apache.org>.
Reamer commented on code in PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#discussion_r1321387286
##########
zeppelin-web-angular/pom.xml:
##########
@@ -80,7 +80,7 @@
</goals>
<configuration>
<skip>${web.e2e.enabled}</skip>
- <arguments>install --no-lockfile</arguments>
+ <arguments>install --legacy-peer-deps</arguments>
Review Comment:
Why is `--legacy-peer-deps` required?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ZEPPELIN-5792][FRONT_BUILD] Remove bower [zeppelin]
Posted by "Armadik (via GitHub)" <gi...@apache.org>.
Armadik commented on PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#issuecomment-2139392787
@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:
`<html>
<body>
<!--StartFragment--><div aria-expanded="true" tabindex="-1" style="box-sizing: border-box; min-width: 0px; min-height: 0px; flex: 0 0 auto; color: rgb(227, 227, 227); font-family: monospace; font-size: 12px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; white-space: normal; background-color: rgb(0, 74, 119); text-decoration-thickness: initial; text-decoration-style: initial; text-decoration-color: initial;"><span class="source-code" style="box-sizing: border-box; min-width: 0px; min-height: 0px; font-family: var(--source-code-font-family); white-space: pre-wrap; font-size: var(--source-code-font-size) !important; line-height: 1.2;"><span class="console-message-anchor" style="box-sizing: border-box; min-width: 0px; min-height: 0px; float: right; text-align: right; max-width: 10
0%; margin-left: 4px;"><button class="devtools-link text-button link-style" jslog="Link; context: script-location; track: click" role="link" tabindex="-1" title="https://zeppelin.dev.analytics.crpt.tech/scripts/vendor.e8bbdaad8068699d.js:51" style="box-sizing: border-box; min-width: 0px; min-height: 0px; font: inherit; cursor: pointer; margin: 0px; height: unset; border: none; border-radius: 2px; padding: 0px !important; color: var(--sys-color-primary); background: none; flex: 0 0 auto; white-space: nowrap; text-decoration: underline; outline-offset: 2px; outline: none; word-break: break-all;"><br class="Apple-interchange-newline">vendor.e8bbdaad8068699d.js:51</button> </span><span class="console-message-text" style="box-sizing: border-box; min-width: 0px; min-height: 0px; color: var(--override-error-text-color) !important;">Uncaught <span class="object-value-error source-code" style="box-sizing: border-box; min-width: 0px; min-height: 0px; font-family: var(--source-code-font-family
); white-space: pre-wrap; font-size: var(--source-code-font-size) !important; line-height: 1.2;"><div style="box-sizing: border-box; min-width: 0px; min-height: 0px;">ReferenceError: autoApply is not defined
<span style="box-sizing: border-box; min-width: 0px; min-height: 0px;"> at n._onCloseHandler (<button class="devtools-link text-button link-style" jslog="Link; context: script-location; track: click" role="link" tabindex="-1" title="https://zeppelin.dev.analytics.crpt.tech/scripts/vendor.e8bbdaad8068699d.js:51:134262" style="box-sizing: border-box; min-width: 0px; min-height: 0px; font: inherit; cursor: pointer; margin: 0px; height: unset; border: none; border-radius: 2px; padding: 0px !important; color: var(--sys-color-primary); background: none; flex: 0 0 auto; white-space: nowrap; text-decoration: underline; outline-offset: 2px; outline: none; word-break: break-all;">vendor.e8bbdaad8068699d.js:51:134262</button>)
</span><span style="box-sizing: border-box; min-width: 0px; min-height: 0px;"> at WebSocket.<anonymous> (<button class="devtools-link text-button link-style" jslog="Link; context: script-location; track: click" role="link" tabindex="-1" title="https://zeppelin.dev.analytics.crpt.tech/scripts/vendor.e8bbdaad8068699d.js:21:5609" style="box-sizing: border-box; min-width: 0px; min-height: 0px; font: inherit; cursor: pointer; margin: 0px; height: unset; border: none; border-radius: 2px; padding: 0px !important; color: var(--sys-color-primary); background: none; flex: 0 0 auto; white-space: nowrap; text-decoration: underline; outline-offset: 2px; outline: none; word-break: break-all;">vendor.e8bbdaad8068699d.js:21:5609</button>)</span></div></span></span></span></div><div class="" role="group" style="box-sizing: border-box; min-width: 0px; min-height: 0px; flex: 0 0 auto; color: rgb(227, 227, 227); font-family: monospace; font-size: 12px; font-style: normal; font-variant-ligatu
res: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; white-space: normal; background-color: rgb(0, 74, 119); text-decoration-thickness: initial; text-decoration-style: initial; text-decoration-color: initial;"><span class="monospace stack-preview-container width-constrained" style="display: inline-block; --monospace-font-size: 12px; --monospace-font-family: monospace; --source-code-font-size: 12px; --source-code-font-family: monospace; width: 1497px; box-sizing: border-box; min-width: 0px; min-height: 0px; font-family: var(--monospace-font-family); font-size: var(--monospace-font-size) !important;">
| n._onCloseHandler | @ | vendor.e8bbdaad8068699d.js:51
-- | -- | -- | --
| (анонимная) | @ | vendor.e8bbdaad8068699d.js:21
</span></div><!--EndFragment-->
</body>
</html>`
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on pull request #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
Posted by "jongyoul (via GitHub)" <gi...@apache.org>.
jongyoul commented on PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#issuecomment-1732996912
LGTM as well. BTW, it will help to reduce our compile time :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul merged pull request #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
Posted by "jongyoul (via GitHub)" <gi...@apache.org>.
jongyoul merged PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] jongyoul commented on a diff in pull request #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
Posted by "jongyoul (via GitHub)" <gi...@apache.org>.
jongyoul commented on code in PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#discussion_r1321624298
##########
pom.xml:
##########
@@ -111,8 +111,8 @@
<scalacheck.version>1.12.5</scalacheck.version>
<!-- frontend maven plugin related versions-->
- <node.version>v12.3.1</node.version>
- <npm.version>6.9.0</npm.version>
+ <node.version>v14.17.0</node.version>
Review Comment:
I also wonder :-) but maybe the author tried to build it from the newer one and found a "relatively" newer version that can build the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zeppelin] pan3793 commented on a diff in pull request #4651: [ZEPPELIN-5792][FRONT_BUILD] Remove bower
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4651:
URL: https://github.com/apache/zeppelin/pull/4651#discussion_r1327966871
##########
zeppelin-web/README.md:
##########
@@ -12,49 +12,49 @@ All build commands are described in [package.json](./package.json)
```sh
# install required depepdencies and bower packages (only once)
-$ npm install -g yarn
-$ yarn install
+$ npm install -g npm
Review Comment:
I think the change is fine.
There are bunch of tools state they are a better replacement of `npm`, including `yarn`, `pnpm` etc., but `npm` still be the default one shipped with NodeJS, I think only mention `npm` in guide is a better choice
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@zeppelin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org