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

Drop bower support and compatibility mode #7230

Merged
merged 49 commits into from
Jan 3, 2020
Merged

Conversation

denis-anisimov
Copy link
Contributor

@denis-anisimov denis-anisimov commented Dec 25, 2019

Part of #6857


This change is Reviewable

@@ -405,19 +279,6 @@ private void initErrorTargets(

private static ApplicationRouteRegistry createRegistry(
VaadinContext context) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

MAJOR Remove this unused method parameter "context". rule

return modules.stream()
.map(module -> resolveResource(module, isJsModule)).sorted()
List<String> resolveModules(Collection<String> modules) {
return modules.stream().map(module -> resolveResource(module)).sorted()
Copy link
Collaborator

Choose a reason for hiding this comment

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

MINOR Replace this lambda with a method reference. rule

@mehdi-vaadin mehdi-vaadin moved this from External Reviews to Iteration Reviews in OLD Vaadin Flow ongoing work (Vaadin 10+) Jan 2, 2020
Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

Reviewed 366 of 481 files at r1, 49 of 114 files at r2, 7 of 7 files at r3, 1 of 1 files at r5.
Reviewable status: 25 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)


webpack.config.js, line 1 at r5 (raw file):

/**

This shouldn't be committed. It should not even be created for the root project, only for the actual modules running the plugin.


flow-server/src/main/java/com/vaadin/flow/component/internal/UIInternals.java, line 187 at r5 (raw file):

    private static final Pattern componentSource = Pattern
            .compile(".*/src/vaadin-([\\w\\-]*).html");

left over html pattern


flow-server/src/main/resources/META-INF/VAADIN/config/flow-build-info.json, line 1 at r5 (raw file):

{}

?? flow-server should not define a build-info file.


flow-server/src/test/java/com/vaadin/flow/server/DeploymentConfigurationFactoryTest.java, line 279 at r5 (raw file):

"\"compatibilityMode\": false,",

Remove token.
For readability this shouldn't be formatted.


flow-server/src/test/java/com/vaadin/flow/server/DeploymentConfigurationFactoryTest.java, line 295 at r5 (raw file):

"\"compatibilityMode\": false,",

Remove token


flow-server/src/test/java/com/vaadin/flow/server/DeploymentConfigurationFactoryTest.java, line 313 at r5 (raw file):

"\"compatibilityMode\": false,",

Remove token


flow-server/src/test/java/com/vaadin/flow/server/DeploymentConfigurationFactoryTest.java, line 329 at r5 (raw file):

"\"compatibilityMode\": false,",

Remove token


flow-server/src/test/java/com/vaadin/flow/server/DeploymentConfigurationFactoryTest.java, line 345 at r5 (raw file):

"\"compatibilityMode\": false,",

Remove token


flow-server/src/test/java/com/vaadin/flow/server/DeploymentConfigurationFactoryTest.java, line 390 at r5 (raw file):

"\"compatibilityMode\": true,",

Remove token


flow-server/src/test/java/com/vaadin/flow/server/UnsupportedBrowserHandlerTest.java, line 101 at r2 (raw file):

        VaadinSession.setCurrent(null);
    }
}

new line was removed


flow-server/src/test/java/com/vaadin/flow/server/VaadinServiceTest.java, line 300 at r5 (raw file):

        return service;
    }
}

New line missing


flow-server/src/test/java/com/vaadin/tests/util/MockUI.java, line 73 at r5 (raw file):

createNpmModeUI()

Could be changed to createUI() as there are no other uis to create


flow-tests/test-dev-mode/src/main/java/com/vaadin/flow/uitest/ui/DependencyView.java, line 58 at r5 (raw file):

        add(new Text(
                "This test initially loads a stylesheet which makes all text red, a JavaScript for logging window messages, a JavaScript for handling body click events and an HTML which sends a window message"),
                new Hr(), new HtmlComponent(), new Hr());

HtmlComponent could be left, just remove the import


flow-tests/test-dev-mode/src/main/java/com/vaadin/flow/uitest/ui/template/AttachExistingElementByIdView.java, line 25 at r5 (raw file):

public class AttachExistingElementByIdView extends AbstractDivView {

    // @HtmlImport("frontend://com/vaadin/flow/uitest/ui/template/AttachExistingElementById.html")

Remove annotation


flow-tests/test-dev-mode/src/main/java/com/vaadin/flow/uitest/ui/template/BundledTemplateInTemplateWithIdView.java, line 29 at r5 (raw file):

@Route(value = "com.vaadin.flow.uitest.ui.template.BundledTemplateInTemplateWithIdView", layout = ViewTestLayout.class)
public class BundledTemplateInTemplateWithIdView extends AbstractDivView {

Does this make any sense anymore as we basically are always bundled?


flow-tests/test-lumo-theme/src/main/java/com/vaadin/flow/uitest/ui/lumo/ExplicitLumoTemplateView.java, line 26 at r5 (raw file):

.html

.js


flow-tests/test-lumo-theme/src/main/java/com/vaadin/flow/uitest/ui/lumo/ImplicitLumoTemplateView.java, line 25 at r5 (raw file):

.html

.js


flow-tests/test-material-theme/src/main/java/com/vaadin/flow/uitest/ui/material/MaterialThemedTemplateView.java, line 27 at r5 (raw file):

.html

.js


flow-tests/test-material-theme/src/main/java/com/vaadin/flow/uitest/ui/material/NotThemedTemplateView.java, line 25 at r5 (raw file):

.html

.js


flow-tests/test-root-context/pom.xml, line 102 at r5 (raw file):

                              running vaadin in debug mode -->
                        <systemProperty>
                            <name>vaadin.disableDevServer</name>

This shouldn't be needed as build-frontend will disable the dev server.
Should be removed so we catch any failures with this functionality in build-info usage.


flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/DomListenerOnAttachView.java, line 26 at r2 (raw file):

@Route(value = "com.vaadin.flow.uitest.ui.DomListenerOnAttachView", layout = ViewTestLayout.class)
@HtmlImport("frontend://com/vaadin/flow/uitest/ui/DomListenerOnAttach.html")

All unreviewed should have the htmlImport removed. This sholdn't even compile as the HtmlImport annotation has been deleted.

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Iteration Reviews to Changes Requested Jan 2, 2020
Copy link
Contributor Author

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

Dismissed @vaadin-bot from 4 discussions.
Reviewable status: 21 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador)


webpack.config.js, line 1 at r5 (raw file):

Previously, caalador wrote…

This shouldn't be committed. It should not even be created for the root project, only for the actual modules running the plugin.

Done.


flow-server/src/main/java/com/vaadin/flow/component/internal/UIInternals.java, line 187 at r5 (raw file):

Previously, caalador wrote…

left over html pattern

Done.


flow-server/src/main/resources/META-INF/VAADIN/config/flow-build-info.json, line 1 at r5 (raw file):

Previously, caalador wrote…

?? flow-server should not define a build-info file.

Wrong location. Thanks, should be in the test resources.


flow-server/src/test/java/com/vaadin/flow/server/DeploymentConfigurationFactoryTest.java, line 279 at r5 (raw file):

Previously, caalador wrote…
"\"compatibilityMode\": false,",

Remove token.
For readability this shouldn't be formatted.

Done.


flow-server/src/test/java/com/vaadin/flow/server/DeploymentConfigurationFactoryTest.java, line 295 at r5 (raw file):

Previously, caalador wrote…
"\"compatibilityMode\": false,",

Remove token

Done.


flow-server/src/test/java/com/vaadin/flow/server/DeploymentConfigurationFactoryTest.java, line 313 at r5 (raw file):

Previously, caalador wrote…
"\"compatibilityMode\": false,",

Remove token

Done.


flow-server/src/test/java/com/vaadin/flow/server/DeploymentConfigurationFactoryTest.java, line 329 at r5 (raw file):

Previously, caalador wrote…
"\"compatibilityMode\": false,",

Remove token

Done.


flow-server/src/test/java/com/vaadin/flow/server/DeploymentConfigurationFactoryTest.java, line 345 at r5 (raw file):

Previously, caalador wrote…
"\"compatibilityMode\": false,",

Remove token

Done.


flow-server/src/test/java/com/vaadin/flow/server/DeploymentConfigurationFactoryTest.java, line 390 at r5 (raw file):

Previously, caalador wrote…
"\"compatibilityMode\": true,",

Remove token

Done.


flow-server/src/test/java/com/vaadin/flow/server/UnsupportedBrowserHandlerTest.java, line 101 at r2 (raw file):

Previously, caalador wrote…

new line was removed

Done.


flow-server/src/test/java/com/vaadin/flow/server/VaadinServiceTest.java, line 300 at r5 (raw file):

Previously, caalador wrote…

New line missing

Done.


flow-server/src/test/java/com/vaadin/tests/util/MockUI.java, line 73 at r5 (raw file):

Previously, caalador wrote…
createNpmModeUI()

Could be changed to createUI() as there are no other uis to create

Done.


flow-tests/test-dev-mode/src/main/java/com/vaadin/flow/uitest/ui/DependencyView.java, line 58 at r5 (raw file):

Previously, caalador wrote…

HtmlComponent could be left, just remove the import

The whole point if this view and related test is to check the order of dependencies.
HtmlComponent has no sense without its HtmlImport :
https://github.com/vaadin/flow/blob/master/flow-tests/test-resources/src/main/resources/META-INF/resources/test-files/html/orderedHtmlImport.html

This won't work in this way at all in NPM mode.

There is a separate ticket to make all ITs working after this PR.
This PR is already huge and overcomplicated, so let's extract tests to a separate task.


flow-tests/test-dev-mode/src/main/java/com/vaadin/flow/uitest/ui/template/AttachExistingElementByIdView.java, line 25 at r5 (raw file):

Previously, caalador wrote…

Remove annotation

ITs will be fixed in separate ticket.


flow-tests/test-dev-mode/src/main/java/com/vaadin/flow/uitest/ui/template/BundledTemplateInTemplateWithIdView.java, line 29 at r5 (raw file):

Previously, caalador wrote…

Does this make any sense anymore as we basically are always bundled?

May be .
ITs will be fixed in separate ticket.


flow-tests/test-lumo-theme/src/main/java/com/vaadin/flow/uitest/ui/lumo/ExplicitLumoTemplateView.java, line 26 at r5 (raw file):

Previously, caalador wrote…
.html

.js

Done.


flow-tests/test-lumo-theme/src/main/java/com/vaadin/flow/uitest/ui/lumo/ImplicitLumoTemplateView.java, line 25 at r5 (raw file):

Previously, caalador wrote…
.html

.js

Done.


flow-tests/test-material-theme/src/main/java/com/vaadin/flow/uitest/ui/material/MaterialThemedTemplateView.java, line 27 at r5 (raw file):

Previously, caalador wrote…
.html

.js

Done.


flow-tests/test-material-theme/src/main/java/com/vaadin/flow/uitest/ui/material/NotThemedTemplateView.java, line 25 at r5 (raw file):

Previously, caalador wrote…
.html

.js

Done.


flow-tests/test-root-context/pom.xml, line 102 at r5 (raw file):

Previously, caalador wrote…

This shouldn't be needed as build-frontend will disable the dev server.
Should be removed so we catch any failures with this functionality in build-info usage.

This property meant something else then disable dev server.
But I cannot find it anymore in the sources. So looks like it's removed.


flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/DomListenerOnAttachView.java, line 26 at r2 (raw file):

Previously, caalador wrote…

All unreviewed should have the htmlImport removed. This sholdn't even compile as the HtmlImport annotation has been deleted.

Almost all ITs are disabled.
I'm not able to fix everything alone.
There is a separate ticket to enable ITs back.

Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

Reviewed 61 of 114 files at r2, 24 of 24 files at r6.
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Changes Requested to Reviewer approved Jan 3, 2020
@caalador caalador moved this from Reviewer approved to Changes Requested in OLD Vaadin Flow ongoing work (Vaadin 10+) Jan 3, 2020
@caalador caalador moved this from Changes Requested to Iteration Reviews in OLD Vaadin Flow ongoing work (Vaadin 10+) Jan 3, 2020
Copy link
Contributor

@mehdi-vaadin mehdi-vaadin left a comment

Choose a reason for hiding this comment

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

Reviewed 105 of 481 files at r1, 4 of 114 files at r2, 1 of 7 files at r3.
Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador and @denis-anisimov)


flow-maven-plugin/src/test/java/com/vaadin/flow/plugin/maven/TestComponents.java, line 44 at r5 (raw file):

VaadinBowerComponent

Can we rename this to avoid confusion in the future?


flow-maven-plugin/src/test/resources/github-com-PolymerElements-iron-behaviors-2.0.0.jar, line 0 at r2 (raw file):
Are these jar files still needed? Can we just delete them?


flow-server/src/main/java/com/vaadin/flow/component/UI.java, line 811 at r5 (raw file):

getThemeFor

Why should this method be deleted?


flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/NpmTemplateParser.java, line 50 at r5 (raw file):

DefaultTemplateParser

It seems that this part of Javadoc is still valid. NpmTemplateParser is singleton as well. The class name should be changed.

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Iteration Reviews to Changes Requested Jan 3, 2020
Copy link
Contributor Author

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador and @mehdi-vaadin)


flow-maven-plugin/src/test/java/com/vaadin/flow/plugin/maven/TestComponents.java, line 44 at r5 (raw file):

Previously, mehdi-vaadin (Mehdi Javan) wrote…
VaadinBowerComponent

Can we rename this to avoid confusion in the future?

Done.


flow-maven-plugin/src/test/resources/github-com-PolymerElements-iron-behaviors-2.0.0.jar, line at r2 (raw file):

Previously, mehdi-vaadin (Mehdi Javan) wrote…

Are these jar files still needed? Can we just delete them?

Oh,
!@#$%^&*

Thanks.

Freaking Git rebase loves me so much!
They were removed and rebase reverted it.
This task is killing me.


flow-server/src/main/java/com/vaadin/flow/component/UI.java, line 811 at r5 (raw file):

Previously, mehdi-vaadin (Mehdi Javan) wrote…
getThemeFor

Why should this method be deleted?

Why not ?

I really mean why not : nothing is broken. It means the method is not used from anywhere.
Everything related to Theme should be removed in fact: theme is handled by webpack during making a bundle.
So it happens once at the build run: either in the very beginning of dev mode or by the maven plugin in production.
It means that any programmatic way to retrieve info from Theme is illegal (since it's already too late).
So if one needs this method then he is doing something totally wrong.


flow-server/src/main/java/com/vaadin/flow/component/polymertemplate/NpmTemplateParser.java, line 50 at r5 (raw file):

Previously, mehdi-vaadin (Mehdi Javan) wrote…
DefaultTemplateParser

It seems that this part of Javadoc is still valid. NpmTemplateParser is singleton as well. The class name should be changed.

Kind of quite extra for this PR.
But OK.

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Changes Requested to Reviewer approved Jan 3, 2020
Copy link
Contributor

@mehdi-vaadin mehdi-vaadin left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 241 of 481 files at r1, 21 of 114 files at r2, 5 of 7 files at r3, 22 of 24 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, 15 of 15 files at r9.
Reviewable status: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained


flow-server/src/main/java/com/vaadin/flow/component/UI.java, line 811 at r5 (raw file):

Previously, denis-anisimov (Denis) wrote…

Why not ?

I really mean why not : nothing is broken. It means the method is not used from anywhere.
Everything related to Theme should be removed in fact: theme is handled by webpack during making a bundle.
So it happens once at the build run: either in the very beginning of dev mode or by the maven plugin in production.
It means that any programmatic way to retrieve info from Theme is illegal (since it's already too late).
So if one needs this method then he is doing something totally wrong.

I was just curious ;) It wasn't blocking. Thanks for the answer :)

Copy link
Contributor Author

@denis-anisimov denis-anisimov left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained


flow-server/src/main/java/com/vaadin/flow/component/UI.java, line 811 at r5 (raw file):

Previously, mehdi-vaadin (Mehdi Javan) wrote…

I was just curious ;) It wasn't blocking. Thanks for the answer :)

Alright.

@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 3 issues

  • MAJOR 2 major
  • MINOR 1 minor

Watch the comments in this conversation to review them.

1 extra 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 PrepareFrontendMojo.java#L194: Format string "%s" needs argument 10 but only 9 are provided in com.vaadin.flow.plugin.maven.PrepareFrontendMojo.propagateBuildInfo() rule

@denis-anisimov denis-anisimov merged commit 3062d5e into ccdm Jan 3, 2020
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Reviewer approved to Done - pending release Jan 3, 2020
@denis-anisimov denis-anisimov deleted the 6857-drop-compatibility branch January 3, 2020 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
OLD Vaadin Flow ongoing work (Vaadin ...
  
Done - pending release
Development

Successfully merging this pull request may close these issues.

None yet

4 participants