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

feat!: Move default frontend to src/main #18803

Closed
wants to merge 20 commits into from

Conversation

caalador
Copy link
Contributor

Move frontend folder from root
to src/main.

Closes #18436

Copy link

github-actions bot commented Feb 26, 2024

Test Results

1 094 files  ± 0  1 094 suites  ±0   1h 14m 43s ⏱️ - 4m 39s
6 937 tests ± 0  6 888 ✅ ± 0  49 💤 ±0  0 ❌ ±0 
7 278 runs  +11  7 217 ✅ +11  61 💤 ±0  0 ❌ ±0 

Results for commit 8810e09. ± Comparison against base commit 75297ef.

♻️ This comment has been updated with latest results.

@vaadin-bot vaadin-bot added +1.0.0 and removed +0.0.1 labels Feb 27, 2024
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.

Some minor comments.

Plus the following leftovers for "frontend" are still there:

  • In ExternalDependencyWatcher class the folder is hardcoded
  • DeploymentConfigurationFactoryTest the same, "frontend" hardcoded

Also, I tried this change with the hybrid and Flow apps, and it doesn't break them. Nothing is changed and they work as previously.
So in what cases this is a breaking change?
Can we merge this immediately without smart detection?

@caalador
Copy link
Contributor Author

ExternalDependencyWatcher mentions frontend for metaInfFolder e.g META-INF and not the project frontend folder.
For DeploymentConfigurationFactoryTest the tests mentioning frontend are for the token file contents so the actual folder doesn't matter.

For the unified execution check that the plugin is using the correct code for the frontend folders. it should not work with having frontend in ./frontend

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.

LGTM, but I propose to rebase the second PR onto this, and merge the second PR.

Let me test it locally.

Copy link

sonarcloud bot commented Feb 29, 2024

Quality Gate Passed Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@mshabarov mshabarov closed this Feb 29, 2024
@caalador caalador deleted the issues/18436-move-frontend branch March 1, 2024 04:54
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move /frontend directory under /src/main by default
3 participants