eslint-plugin-import-helpers
eslint-plugin-import-helpers copied to clipboard
willhoney7
Allow unassigned imports
What?
Add a way to allow sorting for unassigned imports. Please check the updated order-imports.md for documentation.
Resolves issues: https://github.com/Tibfib/eslint-plugin-import-helpers/issues/35 https://github.com/Tibfib/eslint-plugin-import-helpers/issues/24
My Usecase
A very common use-case is to have style imports import 'some-style.scss' sorted at the end of the file when building react components. Even after defining a separate group for these files /\.scss$/, the sorting didn't work because we ignore unassigned imports. This change lets the user enable this option if they'd like.
@Tibfib Thank you for the review & the instructions. I'll try & take up the suggestions later in the day & push them.
@Tibfib Pushed the changes suggested & the test is working fine now.
While doing this I also realized that I'll need checks to make this work for 'require' type imports as well, somewhere here & here
I'm not sure how can we detect if a require is unassigned import since there are a lot of checks around require statements which currently exists.
I tried an experiment of running it over this to know the difference:
require('./path');
var path = require('path');
Diff: https://www.diffchecker.com/au7ZJqaI
Unassigned import node, seem to have the type ExpressionStatement. Just want to confirm if I can rely on that before making the change something like:
unassignedImportOptionPassed && node.type === 'ExpressionStatement'
The function isPlainRequireModule is intended to find unassigned/bare "require" imports. I don't follow on why that's insufficient?
@Tibfib Understood, so the sole purpose of that function is to find unassigned imports.
My question was just around to understand, how can I make sure that this new option that I've introduced also takes care of require('./unassigned.scss') like imports & not just imports like import './unassigned.scss' which I've handled already.
@Tibfib Understood, so the sole purpose of that function is to find unassigned imports.
After re-reviewing the code, I don't think this is correct. The intention of the function is to find import statements and then to make sure those import statements are assigned.
My question was just around to understand, how can I make sure that this new option that I've introduced also takes care of
require('./unassigned.scss')like imports & not just imports likeimport './unassigned.scss'which I've handled already.
Yeah, okay, I'm following a bit more now. I'd have to run this code myself to test for sure, but I think you're on the right track for bare "requires". Make sure you've got good test cases and that the existing test cases don't break while using the ExpressionStatement to verify if it's a bare require.
(Sorry about this! I didn't write all of this code so it takes a bit of diving in for me to remember what it does)
@hyoretsu I got a little occupied with my work, I'll probably be able to resume this sometime by the end of this week.
Spent some time on this today. Somewhere I'm having a hard time figuring out is:
Why does the implementation for isStaticRequire & isPlainRequireModule differs.
As far as I can understand we use the former when reading the require within CallExpression function & the other when running the eslint fix.
Whereas In the case of import statements, we're referring to a single function isAllowedImportModule which was pretty easy to deal with.
What I need to work here is to make sure: isPlainRequireModule appropriately checks for Identifier, CallExpression, require, Literal (i.e the checks that exist within that function) both in case of VariableDeclaration (assigned imports) as well as ExpressionStatement (unassigned imports), because the node value varies in both cases like so:
