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

fix(vue): .eslintrc blocks user from commiting #217

Merged
merged 12 commits into from
May 17, 2021
Merged

Conversation

y-lakhdar
Copy link
Contributor

Proposed changes

Removed .eslintrc file from search-token-server package as it does not have any additional effect. Furthermore, the extends property in that eslint file was pointing to an invalid path once added to a ui project.

The /server folder will be linted based on the project configuration (root eslint file)

Testing

E2E tests to ensure the project can be commited without the lint-staged hook to fail.


CDX-320

@github-actions
Copy link
Contributor

Pull Request Report

PR Title

✅ Title follows the conventional commit spec.

{
"extends": "../../.eslintrc",
"rules": {
"node/no-unpublished-import": ["off"]
Copy link
Contributor Author

@y-lakhdar y-lakhdar May 13, 2021

Choose a reason for hiding this comment

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

This rule should not be applied in this package. Since the search token server folder is entirely copied into ui projects (react, vue, angular), all its dependancies should be listed in the package.json

@@ -1,6 +0,0 @@
{
"extends": "../../.eslintrc",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line breaks point to an invalid path once added to a ui project (react, angular, vue)

@@ -7,7 +7,7 @@ const app = express();

app.use(express.json());

app.get<{}, any, {token: string}>(
app.get<Record<string, never>, any, {token: string}>(
Copy link
Member

Choose a reason for hiding this comment

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

Maybe: Record<string, unknown> ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any would work, but not unknown because of this condition: Type 'unknown' is not assignable to type 'string'.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this need to change?

Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

I don't agree with the solution proposed here tbh.
I think we should lint and keep control of the linting rules being applied to the server.

Comment on lines +15 to +16
dotenv: '^9.0.2',
'fs-extra': '^10.0.0',
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's doesn't make sense IMO for a vue project to have those dependency (even concurrently actually) as dependencies . As dev-dependency, sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because the generated project depends on these 2 packages.
Currently we were lucky because these packages are available though other packages, hence why it never failed.

@@ -7,7 +7,7 @@ const app = express();

app.use(express.json());

app.get<{}, any, {token: string}>(
app.get<Record<string, never>, any, {token: string}>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this need to change?

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

Successfully merging this pull request may close these issues.

3 participants