Exclude pnpm from managing itself #7125

denis-anisimov · 2019-12-10T12:39:07Z

This change is

caalador · 2019-12-10T12:48:36Z

Should we instead use the self-installer so that npm would not be in the mix at all: https://github.com/pnpm/self-installer#readme

caalador

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 377 at r1 (raw file):

            boolean failOnAbsence) {
        // First try local pnpm JS script if it exists
        File npmInstalled = new File(baseDir, PNMP_INSTALLED_BY_NPM);

We should probably not use node_modules/pnpm/bin/pnpm.js but instead use node_modules/.bin/pnpm and node_modules/.bin/pnpm.cmd

denis-anisimov

That means we have to either download the file somehow or bundle it with Flow.

I don't really like both approaches.

The current way ( even though it's hacky ) requires standard toolset: npm only.

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @caalador)

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 377 at r1 (raw file):

Previously, caalador wrote…

We should probably not use node_modules/pnpm/bin/pnpm.js but instead use node_modules/.bin/pnpm and node_modules/.bin/pnpm.cmd

I don't think it matters.

node_modules/.bin/pnpm refers to node_modules/pnpm/bin/pnpm.js.

node_modules/pnpm/bin/pnpm.js is used once and completely removed to avoid problems with managing pnpm itself.
So node_modules/.bin/pnpm just can't be used for normal run.
It's needed only once to install pnpm via pnpm .

And for this only one run I don't see preference.

vaadin-bot · 2019-12-10T13:27:27Z

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java

                }
+                tempFile.delete();


Do something with the "boolean" value returned by "delete".

vaadin-bot · 2019-12-10T13:27:28Z

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java

-            } finally {
-                if (process != null) {
-                    process.destroyForcibly();
+                packageJson.delete();


Exceptional return value of java.io.File.delete() ignored in com.vaadin.flow.server.frontend.FrontendUtils.ensurePnpm(String, boolean)
Do something with the "boolean" value returned by "delete".

vaadin-bot · 2019-12-10T13:27:29Z

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java

+             * dangerous to have it there since it's not able to manage itself
+             * properly. Let's remove it from "node_modules".
+             */
+            new File(baseDir, PNMP_INSTALLED_BY_NPM_FOLDER).delete();


Do something with the "boolean" value returned by "delete".

vaadin-bot · 2019-12-10T13:27:30Z

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java

+             */
+            new File(baseDir, PNMP_INSTALLED_BY_NPM_FOLDER).delete();
+            // remove package-lock.json which contains pnpm as a dependency.
+            new File(baseDir, "package-lock.json").delete();


Do something with the "boolean" value returned by "delete".

vaadin-bot · 2019-12-10T13:27:32Z

SonarQube analysis reported 5 issues

1 major
4 minor

Watch the comments in this conversation to review them.

denis-anisimov

By the way:

self installer can be installed using npm : npm i @pnpm/self-installer
one it's installed the script can be run via node node_modules/\@pnpm/self-installer/install.js
this command downloads pnpm but then it doesn't do anything for me.
documentation says that by default it's installed into PNPM_DEST=node_modules/pnpm . This location causes the problems and this is what I'm trying to avoid.
Can be used another location of course: but that's what I'm exactly trying to avoid: make a decision which location should be used. With this PR pnpm moves the pnpm to a special directory which doesn't collide with pnpm structure for sure. And I don't know whether it's the case with any other directory.

So I think it has no sense to use self-installer.

Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

caalador

Yeah. The quick look made it seem like it would be a good choice to not have to use npm for the install, but it doesn't seem to work as expected and I got a user global install with it.

Reviewed 2 of 2 files at r1.
Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 385 at r1 (raw file):

        if (!addPnpmCommand(baseDir, otherNpmMovedLocation, returnCommand)
                && !addPnpmCommand(baseDir, npmMovedLocation, returnCommand)
                && !addPnpmCommand(baseDir, npmInstalled, returnCommand)) {

For clarity on what is happening I would move the 3 file checks to an own method Optional<File> getLocalPnpm that would return the valid local install so that the if would be easier to read e.g. if(localFile.isPresent()l) { addPnpmCommand(localFile); } else{}

denis-anisimov

Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador and @denis-anisimov)

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 385 at r1 (raw file):

Previously, caalador wrote…

For clarity on what is happening I would move the 3 file checks to an own method Optional<File> getLocalPnpm that would return the valid local install so that the if would be easier to read e.g. if(localFile.isPresent()l) { addPnpmCommand(localFile); } else{}

Yeah, that's much better.
Thanks.

caalador

Reviewed 1 of 1 files at r2.
Dismissed @vaadin-bot from 4 discussions.
Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained

* Avoid clean up if PNPM is used (#7095) Fixes #7083 * Iterate over a copy for UI internals listeners (#7103) Avoids ConcurrentModificationException if firing an event causes listeners to be added or removed. * Convert lambda exps to simple loops to fix Java 11 build (#7102) * Fix command expectations to expect a local install (#7100) Test run against a local installation of pnpm test was expecting a global installation to be used. * Highlight drop target when dragged on child Fixes #7108 ..but does not fix case where child is a drop target too and drag moves back to parent drop target -> the class name for parent element is not reapplied. * Make nested drop target highlighting work Fixes #7109 * Allow adding a local js module through Page.addJsModule (#6933) * Deny adding a local js module through Page.addJsModule * Deprecate Page.addJsModule(url, loadMode) * Remove old "flow-deps" dependency/package-lock if PNPM is used (#7117) Part of #7113 * Fix using host value from the request (#7115) When using the host value from the request header we need to add the transport as host only contains the server and port. In the off case that we get the scheme as a header we should use that. * Exclude pnpm from managing itself (#7125) * Use recommended API to get chrome capabilities (#7138) DesiredCapabilities.chrome will be deprecated and removed. Current usage results in logged warnings "Using `new ChromeOptions()` is preferred to `DesiredCapabilities.chrome()`" https://github.com/SeleniumHQ/selenium/blob/selenium-3.141.59/java/client/src/org/openqa/selenium/remote/DesiredCapabilities.java#L115-L118 * Add explicit "crossorigin" attribute to bundle script tag in bootstrap (#7124) * Use Element API for traversing children to call the onEnabledStateChanged method (#7131) Fixes #7085 * Deprecate isIOS method and make it in ExtendedClientDetails (#7139) Fixes #6671 * Added missing constructor with scanNestedDefinitions option (#7099) Cherry pick from: vaadin/framework#11801 * Upgrade webpack plugins to fix npm audit warnings (#7122) Upgrade compression-webpack-plugin dependency Upgrade copy-webpack-plugin version Upgrade webpack dependencies versions * Only sending to server should be postponed - Update value in StateTree on the client side immediately - Defer execution only sending a new value to the server Fixes #7128 * Add IT test * Remove IT test which doesn't reproduce the issue * Handle possible array (#7159) If building sourcemaps for webpack the assets array will contain an array of bundles instead of a single string. Fixes #7158 * Workarounds for Safari 10 <script nomodule> and scope bugs (#7165) * Unregister listener when disconnecting web component (#7061) * Make links focusable (#7155) * Fix broken mobile dnd polyfill for ipad on iOS13 Fixes #7123 * Copy when jar (#7175) If packaging the project as a jar then we will copy the frontend files during prepare-frontend. This way running the development mode jar should have all frontend files available. Fixes #6994 * Fix running pnpm for a local installation (#7185) * Fix ElementStateProvider serialization (#7192) Add readResolve method for singleton class handing during (de)serialization of state providers. Fixes #7190 * Pin pnpm verion to 4.5 and remove workaround for 4.3.3 (#7191) Fixes #7183 * Pass query parameters to the new location (#7189) * Update chromedriver to v79 (#7181) * Assert chrome version 79 (#7201) * Only clear all if value changes. (#7212) If we remove all when the innerHTML value stays the same we will loose the innerHTML on the client as the value is not re-sent, but is still cleared from the client. Fixes #4644 * Don't add abstract error targets (#7215) Filter out any abstract classes from error navigation targets as they can't be instantiated anyway. Fixes vaadin/spring#530 * Add onclick to error window (#7217) Now the webpack error window can be closed by clicking it. Fixes #7211 * Error message for failing imports should be tool dependent (#7216) Fixes #7182 * Combine all constructors together Fixes #7148 * log: debug the list of dependencies and imports found in classes (#7221) Co-authored-by: Denis <denis@vaadin.com> Co-authored-by: Leif Åstrand <legioth@gmail.com> Co-authored-by: Mehdi Javan <32511762+mehdi-vaadin@users.noreply.github.com> Co-authored-by: caalador <mikael.grankvist@gmail.com> Co-authored-by: Pekka Hyvönen <pekka@vaadin.com> Co-authored-by: Bogdan Udrescu <bogdanudrescu@users.noreply.github.com> Co-authored-by: Guille <alvarezguille@users.noreply.github.com> Co-authored-by: Johannes Eriksson <joheriks@vaadin.com> Co-authored-by: Serhii Kulykov <iamkulykov@gmail.com> Co-authored-by: Muammer Yucel <muammer.yucel.82@gmail.com> Co-authored-by: Giovanni Lovato <giovanni@lova.to> Co-authored-by: Manuel Carrasco Moñino <manolo@apache.org>

Exclude pnpm from managing itself

b53bdc6

caalador requested changes Dec 10, 2019

View reviewed changes

denis-anisimov commented Dec 10, 2019

View reviewed changes

vaadin-bot reviewed Dec 10, 2019

View reviewed changes

vaadin-bot added the +0.0.1 label Dec 10, 2019

denis-anisimov commented Dec 10, 2019

View reviewed changes

caalador requested changes Dec 11, 2019

View reviewed changes

denis-anisimov commented Dec 11, 2019

View reviewed changes

Improve the code for local pnpm script detection

2c4f330

caalador approved these changes Dec 11, 2019

View reviewed changes

denis-anisimov merged commit 815b96c into master Dec 11, 2019

denis-anisimov deleted the avoid-pnpm-dependency branch December 11, 2019 13:35

denis-anisimov added this to the 2.2.0.alpha7 milestone Dec 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude pnpm from managing itself #7125

Exclude pnpm from managing itself #7125

denis-anisimov commented Dec 10, 2019 •

edited by mehdi-vaadin

Loading

caalador commented Dec 10, 2019

caalador left a comment

denis-anisimov left a comment

vaadin-bot Dec 10, 2019

vaadin-bot Dec 10, 2019

vaadin-bot Dec 10, 2019

vaadin-bot Dec 10, 2019

vaadin-bot commented Dec 10, 2019

denis-anisimov left a comment

caalador left a comment

denis-anisimov left a comment

caalador left a comment

Exclude pnpm from managing itself #7125

Exclude pnpm from managing itself #7125

Conversation

denis-anisimov commented Dec 10, 2019 • edited by mehdi-vaadin Loading

caalador commented Dec 10, 2019

caalador left a comment

Choose a reason for hiding this comment

denis-anisimov left a comment

Choose a reason for hiding this comment

vaadin-bot Dec 10, 2019

Choose a reason for hiding this comment

vaadin-bot Dec 10, 2019

Choose a reason for hiding this comment

vaadin-bot Dec 10, 2019

Choose a reason for hiding this comment

vaadin-bot Dec 10, 2019

Choose a reason for hiding this comment

vaadin-bot commented Dec 10, 2019

denis-anisimov left a comment

Choose a reason for hiding this comment

caalador left a comment

Choose a reason for hiding this comment

denis-anisimov left a comment

Choose a reason for hiding this comment

caalador left a comment

Choose a reason for hiding this comment

denis-anisimov commented Dec 10, 2019 •

edited by mehdi-vaadin

Loading