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: Upgrade and enable svelte-core module #10958

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

vishal423
Copy link

closes #10910

@vishal423 vishal423 marked this pull request as ready for review September 24, 2024 05:46
.addScript(scriptKey("test"), scriptCommand("npm run test:watch"))
.addScript(scriptKey("test:coverage"), scriptCommand("vitest run --coverage"))
.addScript(scriptKey("test:watch"), scriptCommand("vitest --"))
.addScript(scriptKey("check"), scriptCommand("svelte-kit sync && svelte-check --tsconfig ./jsconfig.json"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should keep the script structure from angular/react/vue modules (to be able to use TikUi)

Copy link
Author

Choose a reason for hiding this comment

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

Feel free to push changes to this branch, as needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I won't :). Those scripts needs to be updated otherwise it will break the current work on frontend modules and we won't be able to integrate styles in svelte and svelte module will be broken, again.

I don't care about svelte integration so I won't contribute on that but I do care about frontend modules moving forward :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@DamnClin : Don't worry, I'll take the charge of harmonizing the svelte client with other clients :) @vishal423 is helping us here to bring back the svelte client to life ;)

public JHipsterModule buildSvelteModule(JHipsterModuleProperties properties) {
Assert.notNull("properties", properties);

//@formatter:off
return moduleBuilder(properties)
.preCommitActions(stagedFilesFilter("{src/**/,}*.ts"), preCommitCommands("eslint --fix", "prettier --write"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep typescript.
Enforcing a strongly typed language, that allows Type Driven Development, is one of the opinionated view of jhipster-lite.

Copy link
Author

Choose a reason for hiding this comment

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

feel free to push changes, as needed

public JHipsterModule buildSvelteModule(JHipsterModuleProperties properties) {
Assert.notNull("properties", properties);

//@formatter:off
return moduleBuilder(properties)
.preCommitActions(stagedFilesFilter("{src/**/,}*.ts"), preCommitCommands("eslint --fix", "prettier --write"))
.preCommitActions(stagedFilesFilter("{src/**/,}*.{js,svelte}"), preCommitCommands("eslint --fix", "prettier --write", "npm run check"))
Copy link
Contributor

Choose a reason for hiding this comment

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

On other client in jhipster-lite, the pre-commit action doesn't check, it just fixes/reformat.

Copy link
Author

Choose a reason for hiding this comment

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

invocation of eslint --fix implicitly checks for lint errors and resolves whatever it can. If you still have lint errors, then, I believe it print those on console and aborts the commit. Inclusion of svelte check is to avoid commit of erroneous svelte code.

.addScript(scriptKey("test:watch"), scriptCommand("vitest --"))
.addScript(scriptKey("check"), scriptCommand("svelte-kit sync && svelte-check --tsconfig ./jsconfig.json"))
.addScript(scriptKey("lint"), scriptCommand("prettier --check . && eslint ."))
.addScript(scriptKey("format"), scriptCommand("prettier --write '{,src/**/,cypress/**/}*.{md,json,js,cjs,svelte,css,html,yml}'"))
Copy link
Contributor

Choose a reason for hiding this comment

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

The svelte module already depends on the prettier module, that setup a prettier:format script, so there's no need to add a new script for formatting.

Copy link
Author

Choose a reason for hiding this comment

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

Since we use a different pattern in pre-commit hook, this script allows reuse of same for explicit invocation

@@ -17,96 +18,108 @@ public class SvelteModuleFactory {

private static final String ENGINES_NEEDLE = " \"engines\":";

private static final JHipsterSource PRIMARY_MAIN_SOURCE = SOURCE.append("src/main/webapp/app/common/primary/app");
private static final JHipsterDestination PRIMARY_MAIN_DESTINATION = to("src/main/webapp/app/common/primary/app");
private static final JHipsterSource PRIMARY_MAIN_SOURCE = SOURCE.append("src/main/webapp/lib/common/primary/app");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep things in the app folder: other modules (e.g. ts-pagination-domainà that generates additional content will generate content in the app folder.

Copy link
Author

Choose a reason for hiding this comment

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

technically it's internal lib containing app specific components. feel free to update, if needed

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.

Remove the svelte module
4 participants