search

ui5-community / generator-ui5-project

Generator for UI5-based web-apps which use the official UI5 tooling and support multiple deployment targets such as the SAP Business Technology Platform
Apache License 2.0
33 stars 25 forks source link

Proposal: Add prettier config #26

Closed lboehm closed 2 years ago

lboehm commented 2 years ago

Hi @IObert,

I've got another proposal, how easy-ui5 could be improved. I would suggest to add prettier to the generated projects. Prettier provides two helpful features:

easy-ui5 generates two npm tasks:

I propose to add prettier checks to the lint task and prettier --write to the lint-fix task.

At our company we're using this configuration and it's quite helpful. Here you can see the output when executing npm run lint:

image

To achieve this behaviour, we would have to do the following things:

If you like my proposal (or the community likes it 🙂) I would provide a PR. But I wanted to ask first, to make sure my PR won't be in vain.

What do you think?

BR, Lukas

vobu commented 2 years ago

+1 in general for the versatile tool prettier. and that's why this repo already uses it :) check out the dev dependencies and the .prettierrc

lboehm commented 2 years ago

Hi @vobu,

sorry, maybe I wasn't clear enough :) I've meant that easy-ui5 should generate apps that use prettier and not that easy-ui5 itself should use prettier. If I'm not mistaken, an app that is generated with easy-ui5 doesn't provide prettier support:

Best regards Lukas

vobu commented 2 years ago

ah, right on. now it's clear. a ui5 prettier extension was on my list for the longest time (both for opinionated xml-view- and js/ts-controller-formatting) - probably something up your bucket?!?

IObert commented 2 years ago

Sounds like a great idea :) Please go ahead and open a PR.

lboehm commented 2 years ago

Hi @IObert & @vobu,

I've created a PR, which works fine for XML and JS. There are a few things to consider:

What do you think?

BR Lukas

IObert commented 2 years ago

Yes, the warnings about the JS doc are a biit ugly but I couldn't suppress them (and they are just warnings). So it's ok to have them around after the PR as well.

What supprises me here is that the command in the screenshot above fails. That shouldn't happen.

vobu commented 2 years ago

first of all thanks for your efforts so far @lboehm!

  • I didn't consider TypeScript, because I don't have a lot TypeScript experience. Propably some using TS more often knows better how to improve the dev experience for TS developers.

to keep the prettier-scope limited, you could simply ignore .ts e.g. in https://github.com/ui5-community/generator-ui5-project/pull/28/files#diff-50a198bb4c5d390a3c61597268f214c1902a8167a29fdd7cd3bf92470bf6c0f5R2

Right now the code that is generated doesn't follow the ESLint and prettier rules

I'd suggest to add eslint-config-prettier and eslint-plugin-prettier as a dev dependeny and add a .eslintrc such as 

{
  "extends": ["prettier"],
  "plugins": ["prettier"],
  "rules": {
    "prettier/prettier": 1 // could be 2 === error as well
  }
}
lboehm commented 2 years ago

Hi @vobu,

here's my response:

Ignoring TypeScript files

to keep the prettier-scope limited, you could simply ignore .ts

I do ignore .ts files, by just checking XML and JS files: Link That sould do the job, right?

Prettier eslint plugin

I'd suggest to add eslint-config-prettier and eslint-plugin-prettier as a dev dependeny and add a .eslintrc such as

Yes, I do that myself as well. But when creating this PR I did some research and found this:

First, we have plugins that let you run Prettier as if it was a linter rule: ... These plugins were especially useful when Prettier was new. By running Prettier inside your linters, you didn’t have to set up any new infrastructure and you could re-use your editor integrations for the linters. But these days you can run prettier --check . and most editors have Prettier support. The downsides of those plugins are: ...

For details have a look here: https://prettier.io/docs/en/integrating-with-linters.html#notes

I personally think that they have a point. I don't want my code to be red or yellow underlined, I just want it to be formatted properly. So I would prefer the way I've implemented it. But of course, we can discuss 🙂

Hi @IObert,

here's my response:

Prettier exit codes

What supprises me here is that the command in the screenshot above fails. That shouldn't happen.

Prettier exits with exit code 1 when a file isn't formatted properly. See:

It behaves like eslint behaves, when eslint finds an error. I personally think it's a good idea, that prettier behaves this way. The benefit is from my point of view, that I can use prettier in pipelines, when it exits with code 1. Otherwise it would be useless for pipelines. And I think from a developer perspective I don't care if the lint tasks fails in VSCode or not. I just want to know if there are linting & styling errors.

What do you think?

Best regards, Lukas

IObert commented 2 years ago

Which files cannot be fixed by prettier?

I understand the js-doc error for eslint. I already tried to fix it by ignoring the first line but somehow yeoman always removes this comment when coping from the template. I'm not sure what the problem is.

But this is just an eslint rule. I would assume that prettier doesn't even notice this.

lboehm commented 2 years ago

No the files can be fixed and they will be fixed when lint-fix is executed. In that case Prettier will exit with status code 0, so no errors will be displayed in the terminal. But when lint is executed, Prettier won't fix those issues, as it just checks. And if it finds any errors it will exit with status code 1, which leeds to errors in the terminal.

IObert commented 2 years ago

I see. I guess that's ok then.

lboehm commented 2 years ago

@vobu I know it's the christmas holidays, so no hurry 🙂 I just wanted to ask what you're thinking about my comment and if we can merge the current branch?

Best regards and of course a happy new year Lukas

vobu commented 2 years ago

Best regards and of course a happy new year

Prosit Neujahr likewise! Will merge your PR as a good start into 2022 :)

vobu commented 2 years ago

erm, a little early with the celebrations ... @lboehm could you please take a loot at the CI errors for your PR #28? https://github.com/ui5-community/generator-ui5-project/runs/4689473965?check_suite_focus=true https://github.com/ui5-community/generator-ui5-project/runs/4689474021?check_suite_focus=true Shouldn't be a biggie, I hope...

lboehm commented 2 years ago

Hi @vobu

okay, this one is a little bit more complicated than it should be. So this post will be a little bit longer. I hope it's understandable.

My proposal how to fix this issue

We have two options for fixing this issue:

  1. We could make sure that the generators always generate perfectly formatted code.
  2. We could run the npm task lint-fix after the app is generated and all the formatting issues will be fixed automatically.

I prefer option 2, because we have different projects, that are generating the app (generator-ui5-project and open-ux-tools). If we would go with option 1, we would have to make sure that every project generates perfectly formatted code. I think this will lead to a lot of frustration for the developers involved 😃 And: If we change something in generator-ui5-project that leads to code style issues, we will find out due to the unit tests. But when there are code style issues in open-ux-tools, the pipeline at open-ux-tools won't fail (cause they don't test for the generator-ui5-project prettier configuration of course). Then they will publish the package leading to failing pipelines in generator-ui5-project, even when there were no changes at generator-ui5-project.

So from my perspective, I think only option 2 is possible. It's quite easy to achieve. I've added this in the end() function and basically it works fine:

image

The problem we are facing

But unfortunately it's not that easy, because we have a problem with the models.js. This code is generated by generator-ui5-project:

sap.ui.define([
    "sap/ui/model/json/JSONModel",
    "sap/ui/Device"
], function (JSONModel, Device) {
    "use strict";

    return {

        createDeviceModel: function () {
            var oModel = new JSONModel(Device);
            oModel.setDefaultBindingMode("OneWay");
            return oModel;
        }

    };
});

This leads to an eslint error due to missing jsdoc comment:

image

Good thing: This issue can be auto fixed. If we let eslint auto fix it, the following code is the result:

sap.ui.define(
    ["sap/ui/model/json/JSONModel", "sap/ui/Device"]
    /**
     * @param {typeof sap.ui.model.json.JSONModel} JSONModel
     * @param {typeof sap.ui.Device} Device
     */,
    function (JSONModel, Device) {
        "use strict";

        return {
            createDeviceModel: function () {
                var oModel = new JSONModel(Device);
                oModel.setDefaultBindingMode("OneWay");
                return oModel;
            },
        };
    }
);

Unfortunately the implementation of @sap/eslint-plugin-ui5-jsdocs doesn't fix the issue correctly. Because afterwards the issue still exists:

image

The problem is, that the comma should be before the jsdoc comment:

image

When the lint-fix command is executed again, the models.js is touched again, leading to this result:

sap.ui.define(
    ["sap/ui/model/json/JSONModel", "sap/ui/Device"],
    /**
     * @param {typeof sap.ui.model.json.JSONModel} JSONModel
     * @param {typeof sap.ui.Device} Device
     */ /**
     * @param {typeof sap.ui.model.json.JSONModel} JSONModel
     * @param {typeof sap.ui.Device} Device
     */
    function (JSONModel, Device) {
        "use strict";

        return {
            createDeviceModel: function () {
                var oModel = new JSONModel(Device);
                oModel.setDefaultBindingMode("OneWay");
                return oModel;
            },
        };
    }
);

That - of course - isn't what we are looking for. To me it looks like a bug in @sap/eslint-plugin-ui5-jsdocs.

Conclusion

As described above, we should run lint-fix to solve code style issues. The only thing that's questionable is, how we should handle the @sap/eslint-plugin-ui5-jsdocs bug. I think we have to fix the models.js in this repo so it has a valid jsdoc comment. And we have to do the same at open-ux-tools.

@vobu:

  1. Is running lint-fix after app generation okay? (if yes, I would fix the models.js error in generator-ui5-project and open an issue in open-ux-tools to fix it there as well.)
  2. Should we then wait for open-ux-tools to add the jsdoc comment and publish a new npm package version? Or should we add the models.js to the .prettierignore file as long as the issue isn't fixed at the open-ux-tools, so we could merge this PR?

What do you think?

Best regards Lukas

PS: Does someone have contact to the people releaseing @sap/eslint-plugin-ui5-jsdocs? Maybe they could fix the bug? (But we shouldn't wait for them fixing the bug. Either way, we should fix the models.js)

lboehm commented 2 years ago

@vobu Sorry, I had some time and didn't wait for an answer 😃 I've provided a commit that:

  1. runs lint-fix after the project is generated
  2. adds the models.js to the .prettierignore file

This way - in theory - it should generate projects where the test tasks doesn't fail. Somehow the unit tests still fail. I will dig deeper when I find time. I think I've got quite busy times ahead. But I will come back to this issue.

PS: Of course modifying the .prettierignore file is a little bit hacky. If you don't care, I think you could merge, when the tests run successfully. If you care, just keep the PR open and I will undo it, when the issue is fixed at open-ux-tools.

lboehm commented 2 years ago

@vobu Okay, I've identified the root cause. Problem is that Yeoman doesn't execute npm install when executed in a unit test (see this link). And if npm install was skipped, we can't call the lint-fix command because it's missing the relevant npm packages.

To fix it I did the following things:

  1. I only execute lint-fix in the end() function when installation wasn't skipped (see this line of code). So if it's executed in an unit test and installation is skipped, the lint-fix is skipped as well. If it's not executed in an unit test, it runs the installation. This solves the problem that installation fails when executed in unit tests, because eslint isn't found (link).
  2. To fix the failure of the test task in unit tests. I've added the npm run lint-fix command after the project installation (see this line of code.

So I think there's just one thing left (as I wrote above):

As open-ux-tools is generating a models.js file that can't be auto fixed properly (see my comment), I've added it to the .prettierignore file (see this line of code). This isn't beautiful. I hope it will be fixed in the near future in the open-ux-tools. At least I've created a issue over there.

If you don't like that the models.js is added to the .prettierignore, just keep this PR open and I will remove it when the issue is fixed at open-ux-tools. If you don't care, you can merge. In this case, I will create a new PR to remove the file from .prettierignore when the issue is fixed.

BR Lukas

bd82 commented 2 years ago

Hello.

Have you considered using npm-patch-package to implement a "hot-fix" for the @sap/eslint-plugin-ui5-jsdocs npm package?

I have used it in the past to avoid having to wait on upstream changes or to allow consumers to immediately "obtain" a fix without waiting for an official release.

bd82 commented 2 years ago

@lboehm @vobu

As a different workaround is it possible to temporarily disable the @sap/eslint-plugin-ui5-jsdocs rule in the generated projects?

lboehm commented 2 years ago

@bd82 I didn't know about the hot fix possibility. I will have a look into it, when I find time. Thanks for the information!

Disabling the rule completely would be an option. It's as dirty as my proposal to add the models.js to the ignore-file 😃 I think I would prefer adding the models.js to the ignore-file, because it only has a negative impact on the models.js file. Disabling the rule at all has an impact on all files.

But let's wait what @vobu says 🙂

bd82 commented 2 years ago

Hi @lboehm

I am wondering: Do these generated yeoman templates also use the UI5 TypeScript definitions to enable auto-complete in JS code?

The reason I'm asking is because that is the main reason this UI5-JSDocs eslint plugin exists, to add the needed type information to the UI5 import syntax (define) so it could be "linked" with the actual type information and provide auto-complete / semantic checks via the TypeScript language server (even for JS code).

bd82 commented 2 years ago

I see it is supported under a conditional branch:

vobu commented 2 years ago

I am wondering: Do these generated yeoman templates also use the UI5 TypeScript definitions to enable auto-complete in JS code?

yes, they do - it's already in the live project: https://github.com/ui5-community/generator-ui5-project/blob/f81175fa067e38c4373fe26d8be0a12216407fdd/generators/app/index.js#L118

vobu commented 2 years ago

@bd82 I didn't know about the hot fix possibility. I will have a look into it, when I find time. Thanks for the information!

Disabling the rule completely would be an option. It's as dirty as my proposal to add the models.js to the ignore-file 😃 I think I would prefer adding the models.js to the ignore-file, because it only has a negative impact on the models.js file. Disabling the rule at all has an impact on all files.

But let's wait what @vobu says 🙂

not that i got fed wisdom with a spoon 🤣 , but here's my 2 cents: reason I didn't mention any patch-up option like patch-package is that I had high hopes to get the SAP colleagues maintaining the eslint-plugin to Open Source it so it becomes a community asset While I still nourish that hope, it won't happen soon as you @bd82 explained. So the best short-term mitigation plan would be two-fold:

lboehm commented 2 years ago

@vobu Perfect, thanks a lot for your support!

vobu commented 2 years ago

@vobu Perfect, thanks a lot for your support!

sure thing, OS #UI5 community FTW! now that the JSDoc is fixed in Open UX Tools, would you send in another PR for taking out models.js from the ignore file?

lboehm commented 2 years ago

Hi @vobu, yes I will provide a PR in the next days :)