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

Fix: don't use deprecated API #11689

Merged
merged 3 commits into from May 10, 2019

Conversation

Projects
None yet
4 participants
@mysticatea
Copy link
Member

commented May 8, 2019

What is the purpose of this pull request? (put an "X" next to item)

[X] Other, please explain

What changes did you make? (Give an overview)

This PR fixes relative-module-resolver.js to not use deprecated API.

In Node.js v12.2.0, Module.createRequireFromPath() function has been deprecated and replaced by Module.createRequire() function because it newly accepts URL as well.

Is there anything you'd like reviewers to focus on?

Nothing in particular.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Does our polyfill actually support the functionality of createRequire? It seems like we only support paths but not URLs.

@mysticatea

This comment has been minimized.

Copy link
Member Author

commented May 8, 2019

I think that our use case handles only file paths. The second parameter relativeToPath is an absolute path which was processed by path module.

The purpose of this PR is just avoiding of use of deprecated API.

@not-an-aardvark
Copy link
Member

left a comment

Ok, that sounds fine -- I just want to avoid having the polyfill be misleading.

Show resolved Hide resolved lib/util/relative-module-resolver.js Outdated
@platinumazure

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Is there a way we could use no-restricted-syntax or a custom rule to try to catch the URL case? (I assume this would be difficult but I have to ask.)

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented May 8, 2019

This would obviously be a much bigger change, but I feel like maybe we're getting to the point where we should just start using TypeScript in ESLint core rather than relying on JSDoc annotations and case-specific linting rules.

Update lib/util/relative-module-resolver.js
Co-Authored-By: Teddy Katz <teddy.katz@gmail.com>
@mysticatea

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

That, starting using TypeScript, is a nice idea to me :)
But, for the dogfooding of the basic set, we might be able to consider //@ts-check feature with JavaScript.

@g-plane

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Will migrate the codebase of ESLint to TypeScript? Or did I misunderstand something?

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented May 9, 2019

I was just bringing it up as an idea, but I don't think we're migrating anything as a result of this issue. (It would probably be better to have a more complete discussion before deciding to migrate anything.)

@g-plane

This comment has been minimized.

Copy link
Member

commented May 9, 2019

I don't think we're migrating anything as a result of this issue.

That's right. I just clarified it.

Merge branch 'master' into fix-create-require
# Conflicts:
#	lib/util/relative-module-resolver.js

@not-an-aardvark not-an-aardvark merged commit aae6f65 into master May 10, 2019

5 checks passed

commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details

@not-an-aardvark not-an-aardvark deleted the fix-create-require branch May 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.