Skip to content
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

Add import extension rule #292

Merged
merged 1 commit into from Feb 21, 2022
Merged

Add import extension rule #292

merged 1 commit into from Feb 21, 2022

Conversation

Pytal
Copy link
Contributor

@Pytal Pytal commented Feb 9, 2022

Warn when file extensions are not used on imports paths with the import/extensions rule, this is to stay up-to-date with the ecosystem e.g. Node.js 14 docs

In future we may want to set this to error

Also see nextcloud-libraries/webpack-vue-config#297

@Pytal Pytal added the enhancement New feature or request label Feb 9, 2022
@Pytal Pytal requested a review from a team February 9, 2022 19:10
@Pytal Pytal self-assigned this Feb 9, 2022
@Pytal Pytal requested review from PVince81, artonge and skjnldsv and removed request for a team February 9, 2022 19:10
@artonge
Copy link
Contributor

artonge commented Feb 10, 2022

If the best practice is to enforce it, why not do it now?

We can start with a warning and change to an error in a year or so.

@Pytal
Copy link
Contributor Author

Pytal commented Feb 11, 2022

If the best practice is to enforce it, why not do it now?

We can start with a warning and change to an error in a year or so.

Wanted to avoid breaking things by introducing too many changes, so as suggested we can go with a warning instead

index.js Outdated Show resolved Hide resolved
@Pytal Pytal requested a review from artonge February 15, 2022 02:34
Signed-off-by: Christopher Ng <chrng8@gmail.com>
@Pytal
Copy link
Contributor Author

Pytal commented Feb 18, 2022

One more review here? not required in this repo though

@skjnldsv skjnldsv merged commit 423cf3a into master Feb 21, 2022
@skjnldsv skjnldsv deleted the enh/import-extensions-rule branch February 21, 2022 09:06
@raimund-schluessler
Copy link
Contributor

raimund-schluessler commented Apr 14, 2022

With this rule the line

export { default as richEditor } from './richEditor'

is now invalid and one needs to write

export { default as richEditor } from './richEditor/index.js'

Is this the desired behaviour?

@Pytal
Copy link
Contributor Author

Pytal commented Apr 18, 2022

@raimund-schluessler yes, as with the broader ecosystem, e.g. Node.js 14 docs, all import paths should include the file extension with no special treatment of index.js files excluding package imports as specified by the ignorePackages: true option added in this PR

@raimund-schluessler
Copy link
Contributor

@Pytal Thanks for the background information 👍
Should we maybe release this then in the next time? This is a change which will require quite a bit of manual work and I think it would be nice to not mix it with any more bigger changes in the next release. What do you think?

@Pytal
Copy link
Contributor Author

Pytal commented Apr 18, 2022

@raimund-schluessler fine with me as long as this goes in a major as this is breaking :) @skjnldsv fine with releasing this in the next major?

@skjnldsv
Copy link
Contributor

Yep! 👍 😉

@skjnldsv
Copy link
Contributor

This is a change which will require quite a bit of manual work and I think it would be nice to not mix it with any more bigger changes in the next release. What do you think?

Can this be automated with eslint fix? I haven't tried 🤔

@raimund-schluessler
Copy link
Contributor

This is a change which will require quite a bit of manual work and I think it would be nice to not mix it with any more bigger changes in the next release. What do you think?

Can this be automated with eslint fix? I haven't tried 🤔

I run this rule in nextcloud-libraries/nextcloud-vue#2637 already. No fix is offered and npm run lint:fix doesn't do anything unfortunately. I had to adjust everything manually.

@raimund-schluessler
Copy link
Contributor

So.... Who is going to do the release? 🙂 🙈

@Pytal
Copy link
Contributor Author

Pytal commented Apr 20, 2022

So.... Who is going to do the release? 🙂 🙈

Feel free @raimund-schluessler https://github.com/nextcloud/eslint-config#release-new-version 🚀

@raimund-schluessler
Copy link
Contributor

So.... Who is going to do the release? slightly_smiling_face see_no_evil

Feel free @raimund-schluessler https://github.com/nextcloud/eslint-config#release-new-version rocket

Fine with me. But how is the changelog generated?

@skjnldsv
Copy link
Contributor

Fine with me. But how is the changelog generated?

sometimes manually, sometimes with github_changelog_generator

@raimund-schluessler raimund-schluessler mentioned this pull request Apr 20, 2022
1 task
max-nextcloud added a commit to nextcloud/text that referenced this pull request May 3, 2022
As recommended in @nextcloud/eslint-config@8.0.0
nextcloud-libraries/eslint-config#292

Signed-off-by: Max <max@nextcloud.com>
max-nextcloud added a commit to nextcloud/text that referenced this pull request Jul 4, 2022
As recommended in @nextcloud/eslint-config@8.0.0
nextcloud-libraries/eslint-config#292

Signed-off-by: Max <max@nextcloud.com>
max-nextcloud added a commit to nextcloud/text that referenced this pull request Jul 5, 2022
As recommended in @nextcloud/eslint-config@8.0.0
nextcloud-libraries/eslint-config#292

Signed-off-by: Max <max@nextcloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants