Allow unassigned imports

[PezCoder]

What?

Add a way to allow sorting for unassigned imports. Please check the updated order-imports.md for documentation.

Resolves issues: #35 #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.

[PezCoder]

Does eslint compare this output after running '--fix' against this plugin. If so, shouldn't this be re-ordered because when running locally against a file, the fixer does re-order as per the errors mentioned below.

Probably I'm missing something here?

[PezCoder]

@Tibfib Would you please review it whenever you can find the time?

[willhoney7]

Hi @PezCoder!

Thanks so much for this contribution. You're right about that test being off. I pulled down the PR and looks like you've got it working for reporting errors but not for fixing the unassigned imports. I think this is where you'll need to tweak it: https://github.com/Tibfib/eslint-plugin-import-helpers/blob/master/src/rules/order-imports.ts#L193

The "canReorderItems" is reporting false for the unassigned/bare imports. You'll want to pass down the "allow unassignedImports" config setting and have that return true, or something to that effect.

Thanks for your patience with this review. Hopefully you're still interested in getting this in 🙂

[PezCoder]

@Tibfib Thank you for the review & the instructions. I'll try & take up the suggestions later in the day & push them.

[PezCoder]

@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'

[willhoney7]

The function isPlainRequireModule is intended to find unassigned/bare "require" imports. I don't follow on why that's insufficient?

[PezCoder]

@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.

[willhoney7]

I suggest leaving the isPlainImportModule function alone and instead, on this line, add in a check for

I think it would be:

return getOptions(context).unassignedImports === 'allow'` || isPlainRequireModule(node) || isAllowedImportModule(node)

[willhoney7]

not 100% sure on this, I think this would also make it sort on variables or other types of nodes

[PezCoder]

@Tibfib Yeah, I was thinking that too but wanted to make sure I don't let any other node which may not be an import pass through.

[willhoney7]

@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 like import './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]

Any news?

[PezCoder]

@hyoretsu I got a little occupied with my work, I'll probably be able to resume this sometime by the end of this week.

[PezCoder]

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:

[JWess]

I would also love to see this land

[IanTorquato]

News? 👀😬