1247434 - Move all rep component into shared directory

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P2)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

This is a follow up for bug 1211525

All rep components should be moved into devtools/client/shared/components/reps directory.

Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I believe this belongs to the framework component
Honza

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

There's now a new Shared Components, so that's probably what we want.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Ah, yes. Ok, I am working on this now (since bug 1240494 is resolved), yay!

Honza

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1247434.patch

Moving rep files into shared dir.

There are some components still being in the JSON View dir:
* tab, toolbar: let's see if we want to share those or introduce better versions (discussion needed).
* tree-view, will be replaced by new one as soon as bug 1247065 lands.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f587ce86698

Honza

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8720761 [details] [diff] [review]
bug1247434.patch

Ah, I need to add the comments and fix some eslint warnings, updated patch will follow.

Honza

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1247434-eslint.patch

I've done the other changes as another patch so, it's easier to review.
(so, the first patch is still needed)

Honza

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af352a772a49

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8720769 [details] [diff] [review]
bug1247434-eslint.patch

Review of attachment 8720769 [details] [diff] [review]:
-----------------------------------------------------------------

It seems a bit arbitrary to changes the ESLint rules for this directory.  We could live with the `indent` for now if we need to, but I am less sure about the others.  We generally want a consistent style throughout, so changing rules in some directories detracts from that goal.

I think I'll let :pbro make the call as our ESLint wizard

::: devtools/client/shared/components/.eslintrc
@@ +2,5 @@
> +  "globals": {
> +    "define": true,
> +  },
> +  "rules": {
> +    "indent": 0,

We are going to want `indent` checking eventually, so we should decide if we should just indent the module body, or make an eslint plugin to check most indentation but ignore the wrapper.

@@ +3,5 @@
> +    "define": true,
> +  },
> +  "rules": {
> +    "indent": 0,
> +    "no-unused-vars": [2, {"vars": "all", "args": "none"}],

Why does this differ from the rest of DevTools?

@@ +4,5 @@
> +  },
> +  "rules": {
> +    "indent": 0,
> +    "no-unused-vars": [2, {"vars": "all", "args": "none"}],
> +    "padded-blocks": 0,

Why does this differ from the rest of DevTools?

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

It's because of the define method. The current rules are forcing the code to be indented, meaning the entire module, inside the define() method, should be indented. Ugh, that's not what we want, right.

The no-unused-vars is there so, the defined method can use all three arguments, even if they are not used.

Honza

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jan Honza Odvarko [:Honza] from comment #9)
> It's because of the define method. The current rules are forcing the code to
> be indented, meaning the entire module, inside the define() method, should
> be indented. Ugh, that's not what we want, right.
> 
> The no-unused-vars is there so, the defined method can use all three
> arguments, even if they are not used.

We could potentially use a line comment to disable the rule just for the define wrapper.

What about padded-blocks?

Anyway, I want :pbro's opinion either way.

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8720769 [details] [diff] [review]
bug1247434-eslint.patch

Review of attachment 8720769 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/components/.eslintrc
@@ +2,5 @@
> +  "globals": {
> +    "define": true,
> +  },
> +  "rules": {
> +    "indent": 0,

indent is probably one of the most useful rules, not because it catches errors or bad practices, but useful because it prevents loosing time during a review. There's nothing worse than having to report a bunch of review comments about inconsistent indentation.
So we should not turn this rule off.

I've checked the various options offered by this rule and could unfortunately not find anything that would allow to not warn about the way you've indented the module body.
So, indeed, as Ryan said, we could create a new rule that accounts for this.

How do you feel about indenting the body instead? I personally don't mind, but I understand if people don't like this (similar to how you wouldn't indent the <body> tag inside an <html> tag, to make the important part easier to read).

Sorry I don't have any good solution for this, I do think we should simply indent.

@@ +3,5 @@
> +    "define": true,
> +  },
> +  "rules": {
> +    "indent": 0,
> +    "no-unused-vars": [2, {"vars": "all", "args": "none"}],

I agree this shouldn't be different than for the rest of devtools. As much as possible, our rules should be shared across the whole codebase.
Now, I actually think that "args": "none" is a good setting and that we should have it at devtools level.
I can't think of a good example of when this would prevent errors, and it's most often frustrating not to be able to to have, e.g., an unused event parameter in an event handler.
So I'd suggest moving this config to the devtools/.eslintrc file.

@@ +4,5 @@
> +  },
> +  "rules": {
> +    "indent": 0,
> +    "no-unused-vars": [2, {"vars": "all", "args": "none"}],
> +    "padded-blocks": 0,

I understand this is here to further separate the define wrapper.
We consider this define statement as a "technical necessity" to how the module is loaded, not as functional code, and therefore I understand that we want to separate it as much as possible from the "actual" code: hence the padding and the indentation.
Unfortunately again, there's no config options in that rule that would allow padding only around certain blocks. And, similarly to indent, I do think that this rule is an important one that prevents loosing time during a review, and enforces a consistent line rhythm when reading code. So I'm not a fan of turning it off entirely for this directory.

Again, a custom rule would help. But in the meantime (unless you're interested in working on this now), I don't see any other options than to properly indent and pad as eslint requires.

You could add a // eslint-disable-line comment next to the closing parens of the define block, but I don't like that too much, because that would mean adding it in every file that does this + with a comment that explains why we're doing this.

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1247434-eslint.patch

Alright, patch updated.

Honza

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #11)
> Now, I actually think that "args": "none" is a good setting and that we
> should have it at devtools level.
bug 1249627 reported for this

Honza

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8721319 [details] [diff] [review]
bug1247434-eslint.patch

Review of attachment 8721319 [details] [diff] [review]:
-----------------------------------------------------------------

eslint config looks good!

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e635ef19db84

Comment 16

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/fx-team/rev/28398bc8b674563d0ac367750fc5574b131eccff
Bug 1247434 - Move all rep component into shared directory; r=jryans

https://hg.mozilla.org/integration/fx-team/rev/c90f90b4b3eb972c31fb93dd7245b546426cd1d7
Bug 1247434 - fix eslint, add comments and remove Locale; r=jryans, pbrosset

Comment 17

[Lina Butler [:lina]]

https://hg.mozilla.org/mozilla-central/rev/28398bc8b674
https://hg.mozilla.org/mozilla-central/rev/c90f90b4b3eb