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.

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

Always install webpack plugins #9718

Merged
merged 1 commit into from
Jan 4, 2021
Merged

Conversation

caalador
Copy link
Contributor

Plugin installation can be skipped
by defining "update": false, in installed
package.json of plugin under node_modules

Plugin installation can be skipped
by defining `"update": false,` in installed
package.json of plugin under node_modules
@caalador caalador requested review from pleku and mshabarov December 23, 2020 09:09
@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 1 issue

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. MAJOR TaskInstallWebpackPlugins.java#L132: This block of commented-out lines of code should be removed. rule

Copy link
Contributor

@mshabarov mshabarov left a comment

Choose a reason for hiding this comment

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

Would be good to cover this feature by a unit test so that not to accidentally change or lost it.

Copy link
Contributor

@pleku pleku left a comment

Choose a reason for hiding this comment

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

So what this does is it leaves it up for the user to decide if they don't want to reinstall the plugins ?
And then it doesn't matter if we forget to update the version of the plugin when doing changes ?
A bit unsure why this is needed or better as there was no ticket or discussion on this.

A unit test would be nice to make sure nobody accidentally removes the "feature of skipping installing the plugins" but first we should probably agree that do we want to merge this or not.

@caalador
Copy link
Contributor Author

caalador commented Jan 4, 2021

Seeing as the instance of not wanting to install the plugins is more for our development needs on the plugins and not for the end user.
As more people have done development I've noted that due to the version check people have wasted time with wondering why the changes do not work or why they fail after they worked for a while when they have changed the flow version that writes an webpack js file that's uncompatible with the installed plugin (due to not updating plugin version on development time and having TC build a new snapshot that replaces the local snapshot).

Opting out of plugin installation makes it clearer that this is the version used and can be used for making changes before committing to build a new flow-server+maven-plugin instance.

@pleku pleku dismissed mshabarov’s stale review January 4, 2021 12:27

If we see this as an internal feature then I don't think the tests are required

@pleku pleku merged commit 090cda3 into master Jan 4, 2021
@pleku pleku deleted the issues/update_plugin_installation branch January 4, 2021 12:27
caalador added a commit that referenced this pull request Feb 10, 2021
Plugin installation can be skipped
by defining `"update": false,` in installed
package.json of plugin under node_modules
caalador added a commit that referenced this pull request Feb 10, 2021
Plugin installation can be skipped
by defining `"update": false,` in installed
package.json of plugin under node_modules
caalador added a commit that referenced this pull request Feb 10, 2021
Plugin installation can be skipped
by defining `"update": false,` in installed
package.json of plugin under node_modules
caalador added a commit that referenced this pull request Feb 12, 2021
Plugin installation can be skipped
by defining `"update": false,` in installed
package.json of plugin under node_modules
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants