-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Aliasing imports with as
#11283
Comments
I think the guideline here should be something like: avoid using |
I updated the original issue @ycombinator to take your suggestion and be more explicit about when |
For those of you that don't wish to read through my PR that inspired the controversy, the specific situation was doing:
While I do agree with @stacey-gammon that we should generally be avoiding using |
Just for clarification, I think that the example that @stacey-gammon provided is a bad use of
where if it was doing the following, I would see it as a potential good use of
|
Thanks for providing more context @kobelb. I'd be willing to add a caveat to the rule that if we don't have control over the name of the export, and it's very generic, that it's okay to alias. But if it's our code and an import needs to be aliased because it's too generic, then I think that points to a bad name choice for the export. In the specific situation above, you chose the name |
I see there being situations where based on where the code is used, it makes sense to refer to something by different names. Within the esqueue module, it makes sense to refer to just I see it being similar to the general guidance for variable naming. Based on the scope of a variable, different names are more appropriate. I would never name a global variable |
Sounds like we have discovered the real underlying disagreement. :) I really dislike generic names because it can be very difficult to search for usages. Searching for |
I like generic names when used in the right context because I dislike prefixing every class with the same thing, imagine if every named export in the |
Yea, I see what you are saying, and I wouldn't suggest going so far as to prefix everything with I think the decision should be:
|
Agreed
That's very subjective. Clearer rules would be good if we're going to try to enforce something. I would also encourage explicit rules for using |
@w33ble - you thumbs downed the issue but your comment says I'm trying to think of a way to improve consensus since we are nearly split. @tsullivan, @jbudz - if I added a caveat that using import alias' is also acceptable if it's to provide additional context, and you aren't able rename the export (e.g. it's not our code, or too significant of an effort), would it change either of your minds? @w33ble - good point regarding *. It's interesting because that style was suggested by @kjbekkelund as a way to namespace exports instead of using a class with static functions, mentioned in this issue: #10844. But as soon as you use |
My personal preference is to avoid the |
I almost never rely on |
@kjbekkelund said:
Could you give an example (or, even better, a pattern)? Might be good to note it in the style guide as an exception. |
e.g. I could easily see myself doing it in tests, where the code is exported as mentioned in #10844 |
But again: rarely. But I just prefer having the option instead of "nothing is allowed" |
For Cloud UI we
We can then:
which has been a very nice pattern for our reducers. (Very specific to our Redux setup, though. Just an example where "strict rules" might not make sense) |
Example with some explanation by Dan Abramov: https://egghead.io/lessons/javascript-redux-colocating-selectors-with-reducers |
I'm okey with having something along the lines of "you usually shouldn't" in the styleguide, but going to the length of saying "not allowed" or having an eslint fail the build would be a 👎 for me. Later on we can add examples etc. But right now I definitely see potential use-cases for |
OK, I changed back to no vote. It sounds like there's still some vagueness for what people would like to see as an eslint rule. I'd just like to see some consensus around the rules before strict enforcement is added. |
Thanks for the feedback @w33ble and @kjbekkelund - I updated the initial comment to take out mention of any new eslint rules and modified the rule to be: My preference is to add a style guide rule to not alias imports except in the following cases:
|
Even without this being implemented as an eslint rule, I still find the language to be rather restrictive and would prefer we give ourselves some leeway on when to use "as", so I will be leaving my vote as a "No". |
I changed from a yes to a no after reading @kobelb's example. I'd be ok with a guideline that says something along of lines of "don't use |
Wouldn't that recommendation cover usage for readability? It sounds like this wouldn't be a hard rule, but more of a suggestion. Kind of like our "prefer native over lodash" rule. |
@Bargs, Can you be more specific about what you think should qualify as a good reason, or why you disagree with the reasons I gave? I'm not sure which specific example you are referring to since there were quite a few, but if it was the one from the PR - the If everyone agrees on language that provides a bit more leeway, I can adjust it to be I just manually switched over about 1,000 files from default to named, so I feel pretty strongly about not degrading the reason behind the effort. Switching to named and using Also re: static analysis -- @Bargs I think you are going to have to help me with my IDE set up because if I tell intellij to "find usages" of the default export |
I've been thinking more about this situation, and I'm not sure that using
The above is something that I wouldn't normally find offensive in any manner. This isn't being done using an |
The stated benefits of using named exports are easier refactoring and improved readability. As @kobelb showed in the comment above me there are other ways to frustrate the named export rule besides using That only leaves readability, which I think is a good benefit. But we've already identified a couple situations where
I don't think we'll start using
It's because that import path is using a webpack alias which we need to get rid of. |
I used to be in the same boat, but this becomes an issue with React/Redux, if we're going to follow the component/container paradigm and the "everything is a named export" rule. Here's an example container, which wraps a component and passes state as props via the // containers/MyComponent.js
import { connect } from 'react-redux';
import { MyComponent as Component } from '../components/MyComponent';
const mapStateToProps = (state) => {
return {
prop: state.prop,
};
};
export const MyComponent = connect(
mapStateToProps,
)(Component); If I want to name both of these things the same, I can't import the component as So unless we decide that we want to be explicit about container naming and somehow name them differently, All of my containers look like this, btw. It's also quite nice because it makes the This isn't a for or against comment, just pointing out a pattern that may become commonplace. |
Final decision is: our general suggestion is to use import |
…points (#11283) (#202026) ## Summary [Internal link](elastic/security-team#10820) to the feature details With these changes we two new routes: * `/internal/siem_migrations/rules/install`: allows to install a specific set of migration rules * `/internal/siem_migrations/rules/install_translated`: allows to install all translated rules in specified migration Also we connect these two new API calls with the "Install" button within the "migration rules" table and the "Install translated rules" button on the "SIEM migration rules" page. ### Screenshots https://github.com/user-attachments/assets/29390d07-eab5-4157-8958-1e3f8459db09 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Sergi Massaneda <sergi.massaneda@gmail.com>
…points (elastic#11283) (elastic#202026) ## Summary [Internal link](elastic/security-team#10820) to the feature details With these changes we two new routes: * `/internal/siem_migrations/rules/install`: allows to install a specific set of migration rules * `/internal/siem_migrations/rules/install_translated`: allows to install all translated rules in specified migration Also we connect these two new API calls with the "Install" button within the "migration rules" table and the "Install translated rules" button on the "SIEM migration rules" page. ### Screenshots https://github.com/user-attachments/assets/29390d07-eab5-4157-8958-1e3f8459db09 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Sergi Massaneda <sergi.massaneda@gmail.com> (cherry picked from commit 07fbb92) # Conflicts: # x-pack/plugins/security_solution/public/siem_migrations/rules/api/api.ts # x-pack/plugins/security_solution/server/lib/siem_migrations/rules/task/agent/nodes/match_prebuilt_rule/match_prebuilt_rule.ts # x-pack/test/api_integration/services/security_solution_api.gen.ts
…es endpoints (#11283) (#202026) (#202368) # Backport This will backport the following commits from `main` to `8.x`: - [[Rules migration] Add `install` and `install all` migration rules endpoints (#11283) (#202026)](#202026) <!--- Backport version: 8.9.8 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Ievgen Sorokopud","email":"ievgen.sorokopud@elastic.co"},"sourceCommit":{"committedDate":"2024-11-29T17:05:20Z","message":"[Rules migration] Add `install` and `install all` migration rules endpoints (#11283) (#202026)\n\n## Summary\r\n\r\n[Internal link](https://github.com/elastic/security-team/issues/10820)\r\nto the feature details\r\n\r\nWith these changes we two new routes:\r\n\r\n* `/internal/siem_migrations/rules/install`: allows to install a\r\nspecific set of migration rules\r\n* `/internal/siem_migrations/rules/install_translated`: allows to\r\ninstall all translated rules in specified migration\r\n\r\nAlso we connect these two new API calls with the \"Install\" button within\r\nthe \"migration rules\" table and the \"Install translated rules\" button on\r\nthe \"SIEM migration rules\" page.\r\n\r\n### Screenshots\r\n\r\n\r\nhttps://github.com/user-attachments/assets/29390d07-eab5-4157-8958-1e3f8459db09\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by: Sergi Massaneda <sergi.massaneda@gmail.com>","sha":"07fbb925859121d391271a183c8ba00109f53ce1","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Threat Hunting","Team: SecuritySolution","backport:version","v8.18.0"],"number":202026,"url":"https://github.com/elastic/kibana/pull/202026","mergeCommit":{"message":"[Rules migration] Add `install` and `install all` migration rules endpoints (#11283) (#202026)\n\n## Summary\r\n\r\n[Internal link](https://github.com/elastic/security-team/issues/10820)\r\nto the feature details\r\n\r\nWith these changes we two new routes:\r\n\r\n* `/internal/siem_migrations/rules/install`: allows to install a\r\nspecific set of migration rules\r\n* `/internal/siem_migrations/rules/install_translated`: allows to\r\ninstall all translated rules in specified migration\r\n\r\nAlso we connect these two new API calls with the \"Install\" button within\r\nthe \"migration rules\" table and the \"Install translated rules\" button on\r\nthe \"SIEM migration rules\" page.\r\n\r\n### Screenshots\r\n\r\n\r\nhttps://github.com/user-attachments/assets/29390d07-eab5-4157-8958-1e3f8459db09\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by: Sergi Massaneda <sergi.massaneda@gmail.com>","sha":"07fbb925859121d391271a183c8ba00109f53ce1"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/202026","number":202026,"mergeCommit":{"message":"[Rules migration] Add `install` and `install all` migration rules endpoints (#11283) (#202026)\n\n## Summary\r\n\r\n[Internal link](https://github.com/elastic/security-team/issues/10820)\r\nto the feature details\r\n\r\nWith these changes we two new routes:\r\n\r\n* `/internal/siem_migrations/rules/install`: allows to install a\r\nspecific set of migration rules\r\n* `/internal/siem_migrations/rules/install_translated`: allows to\r\ninstall all translated rules in specified migration\r\n\r\nAlso we connect these two new API calls with the \"Install\" button within\r\nthe \"migration rules\" table and the \"Install translated rules\" button on\r\nthe \"SIEM migration rules\" page.\r\n\r\n### Screenshots\r\n\r\n\r\nhttps://github.com/user-attachments/assets/29390d07-eab5-4157-8958-1e3f8459db09\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by: Sergi Massaneda <sergi.massaneda@gmail.com>","sha":"07fbb925859121d391271a183c8ba00109f53ce1"}},{"branch":"8.x","label":"v8.18.0","labelRegex":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT-->
…points (elastic#11283) (elastic#202026) ## Summary [Internal link](elastic/security-team#10820) to the feature details With these changes we two new routes: * `/internal/siem_migrations/rules/install`: allows to install a specific set of migration rules * `/internal/siem_migrations/rules/install_translated`: allows to install all translated rules in specified migration Also we connect these two new API calls with the "Install" button within the "migration rules" table and the "Install translated rules" button on the "SIEM migration rules" page. ### Screenshots https://github.com/user-attachments/assets/29390d07-eab5-4157-8958-1e3f8459db09 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Sergi Massaneda <sergi.massaneda@gmail.com>
…points (elastic#11283) (elastic#202026) ## Summary [Internal link](elastic/security-team#10820) to the feature details With these changes we two new routes: * `/internal/siem_migrations/rules/install`: allows to install a specific set of migration rules * `/internal/siem_migrations/rules/install_translated`: allows to install all translated rules in specified migration Also we connect these two new API calls with the "Install" button within the "migration rules" table and the "Install translated rules" button on the "SIEM migration rules" page. ### Screenshots https://github.com/user-attachments/assets/29390d07-eab5-4157-8958-1e3f8459db09 --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Sergi Massaneda <sergi.massaneda@gmail.com>
During a recent refactor for converting default to named exports the question came up for whether or not, or when, to alias imports with
as
.e.g:
vs
Below, @kobelb brought up the inverse situation:
vs
In that case, I suggest the export should be renamed, if we have control over it, since IMO, it's a code smell indicating an overly generic name choice.
Some background:
My preference is to add a style guide rule to not alias imports except in the following cases:
My main reason is that is allows for inconsistent naming, which is one of the main benefits of enforcing named exports. The biggest benefit of consistent naming is for refactoring and code readability.
If you agree with a style guide rule to avoid the use of
as
unless there is very good reason (see above), 👍 this issue. If you prefer more flexibility and don't wish for this to be in the style guide, 👎 this issue. If you have another preference, comment, and if you don't care either way, then you can go about your day. 😸cc @ycombinator @kobelb
The text was updated successfully, but these errors were encountered: