Summary:
[eslint-typescript/naming-convension rule docs](https://typescript-eslint.io/rules/naming-convention/)
Initially looked into it to enforce UPPER_CASE for top most constants like `const DELAY = 10` which is a standard in js ecosystem, which turned to be more difficuilt(we will still get there).
Turns out we had casing checks disabled for everything but typeLike names. What I did in this diff
- use default setting for eslint rule
`✖ 9890 problems`
- with any propery names
`✖ 8229 problems`
- without checking properies at all
`✖ 3683 problems`
- without checking enum members
`✖ 3231 problems`
- without checking object properties/methods
`✖ 2978 problems`
- allowing PascalCase for variables
`✖ 1657 problems
- allowing PascalCase for functions
`✖ 975 problems
- not checking typeMethod and parameters
`✖ 916 problems`
- allow double underscore before after variable
`✖ 461 problems`
- allow snake_case variables
`✖ 49 problems`
Fix remaining problems.
Future plans. Ban usage of PascalCase for variables that are not components
Reviewed By: LukeDefeo
Differential Revision: D50970193
fbshipit-source-id: d9f3abe6b02c9f7822598c8fa5382f58d067f70e
Summary:
Let's ban the non null assertion syntax `maybeNull!` as it is unsafe and can cause unexpected behavior. Ideally this should be an error or require a comment explaining why the value cannot be nullish. Though, there are 468 occurrences around the codebase and it is not worth it to manually go around fixing it now. Thus I used a warn to still warn developers to avoid the use where possible.
Last week I spend over an hour on debugging something where the code was null-asserted as the developer thought the value could never be null while in reallity it could. Adding a runtime check in this case sounds reasonable.
rule docs
https://typescript-eslint.io/rules/no-non-null-assertion/
Reviewed By: nikoant
Differential Revision: D45773327
fbshipit-source-id: 9e8a40af353ce979f469ffaedd8e777d72500dab
Summary: We have a list of modules that we do not bundle with the plugins, but provide externally to them from Flipper. For the mechanism to work correctly, we have to stop importing from nested paths of these modules.
Reviewed By: mweststrate
Differential Revision: D39776237
fbshipit-source-id: 06eae9bf9d5b11b48d2720bf592bfea749773847
Summary:
A few comments in my rewrite stack got missed, I am addressing them here.
In addition react testing library has been hoisted to the root module in the project and been made available to all sub modules
Reviewed By: mweststrate
Differential Revision: D37712339
fbshipit-source-id: 60984c3d16bd535b0c489570907f097c7d80f634
Summary: The linting exception is no longer needed as we have decapitated React DevTools
Reviewed By: nikoant
Differential Revision: D35283282
fbshipit-source-id: 73f4b14dcd35f6d93b464513fd8480bfa8a0168c
Summary: remove statically analysable life from typescript guidelines and let eslint handle it
Reviewed By: passy
Differential Revision: D33819891
fbshipit-source-id: 3cc3cb512607c3651cd3a9e48228d83bab5bb64a
Summary: Did not find any usage of getQueryFromQueryId.tsx
Differential Revision: D33658515
fbshipit-source-id: 93f0a42ef30de158dbdb3d8d70559a87839522b0
Summary:
This diff adds `types` fields on the compiler config for every project. This way we can make sure that for example node types and packages are not available in flipper-ui-core. Without an explicit types field, all types would be shared between all packages, and implicitly included into the compilation of everything. For the same reason `types/index.d.ts` has been removed, we want to be intentional on which types are being used in which package.
This diff does most of the work, the next diff will fine tune the globals, and do some further cleanup.
As an alternative solution I first tried a `nohoist: **/node_modules/types/**` and make sure every package list explicitly the types used in package json, which works but is much more error prone, as for example two different react types versions in two packages will cause the most unreadable compiler error due to the types not being shared and not literally the same.
Reviewed By: lawrencelomax
Differential Revision: D33124441
fbshipit-source-id: c2b9d768f845ac28005d8331ef5fa1066c7e4cd7
Summary: Since no plugin directly uses adbkit or crc32 anymore, we removed them from the globals. They wouldn't work in the browser anyway
Reviewed By: aigoncharov
Differential Revision: D33019084
fbshipit-source-id: 66ab0756399fdb401c63f5e8271bdd62cb79ab4a
Summary:
Forbid imports from nested directories of flipper-common, flipper-plugin, flipper-ui-core
In the stack of D32926830 I occasionally imported from flipper-plugin/src/... which is not allowed. This should fix any auto-import related issues once and for all.
Reviewed By: timur-valiev
Differential Revision: D32987054
fbshipit-source-id: f19f6278219961ad283cacfec094a6c703ca93f8
Summary: Setting the lint level to error and fixing the last three violations.
Reviewed By: nikoant
Differential Revision: D32109025
fbshipit-source-id: 213ecaa17317f665ac5a192682fa06d182c28581
Summary:
Grey -> gray. "Cancelled" seems quite common in APIs though, so I disabled that.
A few promise cleanups
Reviewed By: aigoncharov
Differential Revision: D31323610
fbshipit-source-id: c8863d995936f451c24eb408fe5c26677187f089
Summary:
That's another thing I comment on a lot and is a mostly avoidable source of
bad perf.
Reviewed By: mweststrate
Differential Revision: D30450577
fbshipit-source-id: bb82d8cbd34956fa790243f59cda09ff9c4e7379
Summary:
I hate when I have to comment on a diff and tell somebody to please not use
Her Majesty's Spelling, so I'll let the computer do the shouting.
Mixing two ways of spelling the same thing just isn't fun. I had to
work with an Android library that insisted on spelling it `colour`,
leading to awkward code like `colour: COLORS.BLUE` which is just annoying
and hard to remember.
Reviewed By: lblasa
Differential Revision: D30015807
fbshipit-source-id: 9f913e72617301273dbe12c60b9cdba8cea05537
Summary: Similarly to previous stack, remove the need to import Electron to write things to clipboard. Introduced linter to prevent future use.
Reviewed By: timur-valiev
Differential Revision: D29661777
fbshipit-source-id: 7bc67ede40b65c5f232b69128f3a423e232ddc1b
Summary:
Added rules to ensure we properly propagate errors from promises.
Also found and fixed a place where promise function parameter was mismatched.
Reviewed By: mweststrate
Differential Revision: D28537820
fbshipit-source-id: b93f44274fc76544049813f645508cb78e432880
Summary:
This checks for unnecessary boolean assignments in JSX,
e.g. `grow={true}` which can just be `grow`.
Saw this because mweststrate commented this on a diff.
Reviewed By: mweststrate
Differential Revision: D28329647
fbshipit-source-id: ed408a0d4977315cdd0ff2d13024bb720c27e24a
Summary: It is quite easy to miss some flipper imports, this lint rule makes them more visible. In the future we can eventually turn this into an error.
Reviewed By: jknoxville
Differential Revision: D28027118
fbshipit-source-id: 817b356634cb78b11b9216ce45429ea7a22f0e4f
Summary:
`console.error(err)` are hard to identify in the codebase especially
as we often don't have reliable stack trace information.
I've already cleaned up a bunch of them manually by going after the most
high-firing ones; this should make it easier to identify the remaining ones.
Reviewed By: jknoxville
Differential Revision: D27913964
fbshipit-source-id: 0ff6624a0c083829846550b40954945d655b7cf6
Summary:
For context see https://fb.workplace.com/notes/470523670998369
This diff introduces the DataSource abstraction, that can store records. If a key is set a key -> record mapping is stored, to make it easy to update existing records using `upsert`, without knowing their exact index.
Internal storage will be slightly altered in upcoming diffs, so don't pay to much attention to that part.
Reviewed By: nikoant
Differential Revision: D25953337
fbshipit-source-id: 1c3b53a2fcf61abaf061946be4af21d2aecc6c6d
Summary: When exploratory testing Flipper, I generally see quite some React key warnings. So it seems that plugin devs often miss them. This diff will configure linting more aggressively to address that (it's not fool proof, but will find the most common cases).
Reviewed By: nikoant
Differential Revision: D26722707
fbshipit-source-id: e0d2b56de2422e1147f52c8e9150d00c7ee64bd2
Summary:
I don't think there's an easy way to do this based on types
which would be ideal ...
So instead I'm checking for
- Importing `remote` from `electron`.
- Accessing `electron.remote`.
You can still hack this by importing `electron`, saving
it to a differently named variable and accessing `remote` on it,
but this should cover all reasonable cases we see in real code.
Reviewed By: mweststrate
Differential Revision: D26453006
fbshipit-source-id: 4b3d223bed43ca3f0d1a4f592ea8f8060a823479
Summary: importing `antd` in a plugin that lives in the Flipper repo will give a int warning. This fixes that, antd is provided by the host package.
Reviewed By: nikoant
Differential Revision: D26073161
fbshipit-source-id: 897357fafce20129f7e12c035ff99cb4870cc814
Summary:
* Removed Flow compilation step
* Removed all `flow` annotations
* Removed all FlowFixMe's
* Removed flow typings for Flipper
* Left flow transpilation (stripping) in babel, in case there is any external user using Flow in his plugin
* Left `eslint-plugin-flowtype` dependencies, as `eslint-config-fbjs` requires it
Reviewed By: passy
Differential Revision: D24755545
fbshipit-source-id: 9c0a7910657fd1cba88294e041bf2bfdf7b565bf
Summary:
I noticed that after the typescript upgrade, I got several weird positives from ESLint (like unused parameters in a type definition, which are obviously always unused, e.g. `type onClick = (e: Event) => void`). After some investigation, it turned out these warnings are generated by eslint, but that those rules should be performaned by typescript/eslint instead. For future reference to which rules this applies:
https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/README.md#extension-rules
Updated the config, and while at it, fixed all warnings in our codebase, except for `react-hooks/exhaustive-deps` warnings, since those require semantic changes.
This reduces the amount of eslint warnings from 86 to 39.
Reviewed By: passy
Differential Revision: D23905630
fbshipit-source-id: 0557708fd9ec6b17840a3c191e7d3baf225bdf23
Summary:
allow-large-files
Updated to typescript 4. Note that this is no new major, the way TS numbers is that 4.0 is simply the number after 3.9 (so they refuse to use 3.10).
Primarily reason is that it allows us to use `/** deprecated */`. although there are definitely more interesting improvements
Reviewed By: passy, nikoant
Differential Revision: D23869445
fbshipit-source-id: 54e251b398b8698d9b18898ce66c3203b32aa356
Summary: Now that the vanilla JS QuickPerformanceLoggerCore was created with the core methods from QPL, we are using this in Flipper so that we have one source of truth
Reviewed By: nubbel
Differential Revision: D23128231
fbshipit-source-id: 16841aab2563509c7184a09ecb8d3f534c53e896
Summary:
Added infra for writing and using custom eslint rules and created a rule for disallowing cross-package file imports. Such imports are anti-pattern and they also break standalone plugin bundling.
We still have a bunch of places where Flipper core references code directly from plugins. I've ignored these places for now and added task T71355623 to revisit them.
Reviewed By: passy
Differential Revision: D22998955
fbshipit-source-id: d04cff8fc115ba1300a7e6830306ec134046e927
Summary:
So far there were 2 types of plugins: `FlipperPlugin` and `FlipperDevicePlugin`. This introduces a third kind: `SandyPluginDefinition`.
Unlike with the old plugins, the export of the module is not directly exposed as the plugin definition. Rather, we use class `SandyPluginDefinition` (instance) that holds a loaded definition and its meta data separately (`PluginDetails`). This means that we don't have to mix in and mutate loaded definitions, and that for unit tests we can avoid needing to provide a bunch of meta data. This also prevents a bunch of meta data existing on two places: on the loaded classes as static fields, and in the meta data field of the loaded class as well. Finally, we can now freely extends the `PluginDetails` interface in flipper, without needing to store it on the loaded classes and we are sure that no naming conflicts are caused by this in the future.
For compatibility with the existing code base, common fields are delegated from the `SandyPluginDefinition` class to the meta data.
Also cleaned up types around plugins a little bit and removed some unnecessary casts.
For all features that reason about plugins in general (such as exports), sandy plugins are ignored for now.
`SandyPluginInstance` is worked out in further diffs
The `instanceof` calls are replaced by a utility function in later diffs.
{F241363645}
Reviewed By: jknoxville
Differential Revision: D22091432
fbshipit-source-id: 3aa6b12fda5925268913779f3c3c9e84494438f8
Summary: Set up an initial library which can (should) be used by plugins in the future.
Reviewed By: jknoxville
Differential Revision: D22019554
fbshipit-source-id: 502b14b34b2c9c117cea377ab6ebbf150e6faee9
Summary:
Had some horrible dev experience caused by VSCode and Jest not agreeing what snapshots should look like, every time saving a test in VSCode, it would break the jest snapshots. After investigating eslint and prettier, turned out VSCode itself was the culprit. Caused by a global user setting, but this makes sure nobody else runs into it :)
Also made sure Jest uses the correct prettier settings (it didn't before).
Reviewed By: passy
Differential Revision: D22114656
fbshipit-source-id: aef6c278a668bce049c96aba95c7a5101ca840ad
Summary:
Enabled linting rules that help to signal making errors with effect dependencies and such.
Fixed all errors, left any warnings generated by the hooks untouched
Reviewed By: nikoant
Differential Revision: D21721497
fbshipit-source-id: 9548453443fa7b663dc4d4289132f388c6697283
Summary: Added eslint rule "no-extraneous-imports" which disallow using modules which are not listed as dependencies in the corresponding package.json. Fixed a bunch of reported errors after the rule applied.
Reviewed By: passy
Differential Revision: D21186848
fbshipit-source-id: 0af9ac4b3fffdfd0ab7c23ae4ff12a3f5989d5e9
Summary:
Quick notes:
- This looks worse than it is. It adds mandatory parentheses to single argument lambdas. Lots of outrage on Twitter about it, personally I'm {emoji:1f937_200d_2642} about it.
- Space before function, e.g. `a = function ()` is now enforced. I like this because both were fine before.
- I added `eslint-config-prettier` to the config because otherwise a ton of rules conflict with eslint itself.
Close https://github.com/facebook/flipper/pull/915
Reviewed By: jknoxville
Differential Revision: D20594929
fbshipit-source-id: ca1c65376b90e009550dd6d1f4e0831d32cbff03
Summary:
Pull Request resolved: https://github.com/facebook/flipper/pull/878
1) Enabled eslint error reporting for unresolved imports and fixed all the errors
2) Enabled eslint for typings (d.ts) and fixed all the errors
Reviewed By: passy
Differential Revision: D20335151
fbshipit-source-id: 7b142281a406b32df0f02a5cd0d7d05eba941acd
Summary:
Pull Request resolved: https://github.com/facebook/flipper/pull/872
Move all the JS code related to desktop app to "desktop" subfolder.
The structure of "desktop" folder:
- `src` - JS code of Flipper desktop app executing in Electron Renderer (Chrome) process. This folder also contains all the Flipper plugins in subfolder "src/plugins".
- `static` - JS code of Flipper desktop app bootstrapping executing in Electron Main (Node.js) process
- `pkg` - Flipper packaging lib and CLI tool
- `doctor` - Flipper diagnostics lib and CLI tool
- `scripts` - Build scripts for Flipper desktop app
- `headless` - Headless version of Flipper app
- `headless-tests` - Integration tests running agains Flipper headless version
Reviewed By: passy
Differential Revision: D20249304
fbshipit-source-id: 9a51c63b51b92b758a02fc8ebf7d3d116770efe9
