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.

Sign up for GitHub

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

Jump to bottom

[Fix] no-unresolved: check import() #2026

Merged
merged 1 commit into from
May 13, 2021
Merged

[Fix] no-unresolved: check import() #2026

merged 1 commit into from
May 13, 2021

Conversation

aladdin-add
Copy link
Contributor

@aladdin-add aladdin-add commented Apr 12, 2021 •
edited by ljharb
Loading

dynamic import has changed in estree

refs: estree/estree#197

Fixes #2024

@aladdin-add aladdin-add changed the title fix: no-unresolved check import() (fixes #2024) wip: fix: no-unresolved check import() (fixes #2024) Apr 12, 2021
@aladdin-add aladdin-add marked this pull request as draft April 12, 2021 09:47
@coveralls
Copy link

coveralls commented Apr 12, 2021 •
edited
Loading

Coverage Status

Coverage increased (+0.9%) to 82.398% when pulling 96ed3c7 on aladdin-add:issue2024 into ab82094 on benmosher:master.

@aladdin-add aladdin-add changed the title wip: fix: no-unresolved check import() (fixes #2024) fix: no-unresolved check import() (fixes #2024) Apr 12, 2021
@aladdin-add aladdin-add marked this pull request as ready for review April 12, 2021 10:15
@aladdin-add aladdin-add force-pushed the issue2024 branch 2 times, most recently from 1dfde11 to 2420b04 Compare April 12, 2021 10:21
@aladdin-add
Copy link
Contributor Author

Require “Allow Edits” / Require “Allow Edits” (pull_request_target)

I didn't see the option in the page. :-( 

@aladdin-add
Copy link
Contributor Author

friendly ping @ljharb

@ljharb
Copy link
Member

ljharb commented May 11, 2021

@aladdin-add i'll take a look soon, but merging this is still blocked on #1986.

@ljharb
Copy link
Member

ljharb commented May 11, 2021

As for the "allow edits" failure; that's because you're making a PR from a fork that's not "aladdin-add", it's "hello-weiran", so due to a github bug, that means you'd need to explicitly give me write perms on https://github.com/hello-weiran/eslint-plugin-import so I can force push to the PR branch.

@ljharb
Copy link
Member

ljharb commented May 12, 2021

This might be a duplicate of #2012.

I'll rebase this after landing that, and keep whatever I can.

@aladdin-add
Copy link
Contributor Author

didn't notice that. seems this one is no longer needed, so it can be closed now.

@aladdin-add aladdin-add reopened this May 12, 2021
@aladdin-add aladdin-add force-pushed the issue2024 branch 9 times, most recently from 2b05677 to 6f46b6f Compare May 13, 2021 02:45
@aladdin-add
Copy link
Contributor Author

the "allow edits" seems working after transfering the repo to "aladdin-add" group.

@aladdin-add
Copy link
Contributor Author

@ljharb ptal~

@aladdin-add aladdin-add force-pushed the issue2024 branch 3 times, most recently from 56c4516 to 46da178 Compare May 13, 2021 03:32
@ljharb ljharb changed the title fix: no-unresolved check import() (fixes #2024) [Fix] no-unresolved: check import() May 13, 2021
@ljharb
Copy link
Member

ljharb commented May 13, 2021

@aladdin-add still looks like there's some test failures; can you take a look?

@aladdin-add
Copy link
Contributor Author

sure, will do!

@aladdin-add
Copy link
Contributor Author

the strange thing is: I was not able to repro it - it's working on my computer.

image
maybe it is related to the "Typescript"?

@aladdin-add aladdin-add force-pushed the issue2024 branch 2 times, most recently from 5be09ba to b7569a6 Compare May 13, 2021 06:26
@ljharb
Copy link
Member

ljharb commented May 13, 2021

What npm version are you using to install it? On node 10+, we're using npm 7 latest in CI.

@aladdin-add
Copy link
Contributor Author

my local env:
node.js v16.1.0
npm: 7.12.1

On node 10+, we're using npm 7 latest in CI.

I don't think so - there was an error when upgrading npm
https://github.com/benmosher/eslint-plugin-import/pull/2026/checks?check_run_id=2573212587#step:3:532

@ljharb
Copy link
Member

ljharb commented May 13, 2021

While that’s concerning, node 15 and 16 would still ship with some version of npm 7. I’ll have to look into that failure separately.

@aladdin-add
Copy link
Contributor Author

aladdin-add commented May 13, 2021 •
edited
Loading

it seems not related to npm. I switched to npm v6.14.13(local env), and the tests was not failing.

@aladdin-add
Copy link
Contributor Author

it turns out the node.js/npm version is not as expected. (tested on my fork)

image

@ljharb
Copy link
Member

ljharb commented May 13, 2021

That's a problem that I thought I'd fixed :-/ i'll take a look at it on master.

@ljharb
Copy link
Member

ljharb commented May 13, 2021

Fixed the tests in master, will rebase now.

@ljharb ljharb closed this May 13, 2021
@ljharb ljharb reopened this May 13, 2021
@ljharb ljharb merged commit 96ed3c7 into import-js:master May 13, 2021
@aladdin-add aladdin-add deleted the issue2024 branch August 3, 2021 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

no-unresolved does not check dynamic import
3 participants
@aladdin-add @coveralls @ljharb