Skip to content

Fix interpreting some external modules being interpreted as internal modules#794

Merged
ljharb merged 1 commit intoimport-js:masterfrom
ephys:master
Mar 3, 2019
Merged

Fix interpreting some external modules being interpreted as internal modules#794
ljharb merged 1 commit intoimport-js:masterfrom
ephys:master

Conversation

@ephys
Copy link
Contributor

@ephys ephys commented Apr 6, 2017 •
edited by ljharb
Loading

Checklist:

  • write tests
  • implement feature/fix bug
  • update docs
  • make a note in change log

Fixes #793

It was fixed by making isExternalPath search for <module_dir>/<package_name> (e.g. node_modules/redux-saga) instead of <module_dir>/<package_name>/<file_name> (e.g. node_modules/redux-saga/effects) in the resolved path.

I'd argue that the new solution is still not optimal, as it will fail if an external module has a main field which links to another external module.

Should we instead just check that the beginning of the resolved path is the resolved path for the external module folder ? I can make a PR with that alternate solution if requested.

You also might want to check the way I implemented the tests as they might not be an optimal solution.

Edit: The build is failing but was already failing before I started writing code, should I be worried about this ? I don't think it's related to anything I did

@ephys @ljharb
[Fix] Fix interpreting some external modules being interpreted as int…
e9544f8 
…ernal modules

Fixes import-js#793.

 - Add skipped test to expect scoped internal packages to be "internal"
@benmosher benmosher requested a review from jfmengels April 12, 2017 11:29
@mastilver
Copy link
Contributor

mastilver commented Jul 7, 2017

@ephys Do you mind rebasing your branch?
I think the tests are more stable now

@orther
Copy link

orther commented Jul 18, 2017

Is there anything Is there anything I can do to help push this along?

@SpainTrain SpainTrain mentioned this pull request Aug 2, 2017
Closed
@coveralls
Copy link

coveralls commented Aug 2, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.002%) to 95.967% when pulling 56cd706 on Ephys:master into dd28130 on benmosher:master.

3 similar comments
@coveralls
Copy link

coveralls commented Aug 2, 2017

Coverage Status

Coverage increased (+0.002%) to 95.967% when pulling 56cd706 on Ephys:master into dd28130 on benmosher:master.

@coveralls
Copy link

coveralls commented Aug 2, 2017

Coverage Status

Coverage increased (+0.002%) to 95.967% when pulling 56cd706 on Ephys:master into dd28130 on benmosher:master.

@coveralls
Copy link

coveralls commented Aug 2, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.002%) to 95.967% when pulling 56cd706 on Ephys:master into dd28130 on benmosher:master.

@ljharb ljharb changed the title Fix issue #793 Fix interpreting some external modules being interpreted as internal modules Aug 2, 2017
@ljharb ljharb requested a review from benmosher August 2, 2017 20:49
@SpainTrain
Copy link

SpainTrain commented Sep 8, 2017

ping-a-doodle-reno

@ljharb
Copy link
Member

ljharb commented Sep 8, 2017 •
edited
Loading

@ephys would you mind rebasing this (on the command line, so there's no merge commits)?

@benmosher / @jfmengels, it'd be great if we could get a look at this within the next week or so, so it can be merged ❤️

@kusmierz
Copy link

kusmierz commented Sep 15, 2017

@ephys any chance to rebase it?

@ephys
Copy link
Contributor Author

ephys commented Sep 15, 2017

@kusmierz Yes, it's on my backlog for this week :) Sorry very busy week as usual

@ephys
Copy link
Contributor Author

ephys commented Sep 15, 2017

Apart from the (unrelated I hope) failed check, all should be good

@SpainTrain
Copy link

SpainTrain commented Oct 20, 2017

@ephys FYI got a changelog conflict

@SpainTrain
Copy link

SpainTrain commented Nov 22, 2017

😿 dreams... 😸

@ljharb ljharb force-pushed the master branch 2 times, most recently from 7a330f5 to e319f78 Compare January 5, 2018 08:09
@coveralls
Copy link

coveralls commented Jan 5, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.002%) to 96.212% when pulling 7a330f5 on Ephys:master into 359a200 on benmosher:master.

1 similar comment
@coveralls
Copy link

coveralls commented Jan 5, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.002%) to 96.212% when pulling 7a330f5 on Ephys:master into 359a200 on benmosher:master.

@coveralls
Copy link

coveralls commented Jan 5, 2018 •
edited
Loading

Coverage Status

Coverage increased (+3.2%) to 97.315% when pulling e9544f8 on Ephys:master into 083bb47 on benmosher:master.

ljharb
ljharb requested changes Jan 5, 2018
View reviewed changes
@krvajal
Copy link

krvajal commented Jul 24, 2018

Is this expect to be merged at some point in the near future?

@ljharb
Copy link
Member

ljharb commented Jul 24, 2018

@krvajal it's still waiting for @ephys to address
#794 (comment)

@krvajal
Copy link

krvajal commented Jul 24, 2018

@ljharb it has been seven month already and no reply. You can either close the PR or allow someone to address the requested changes

@ljharb
Copy link
Member

ljharb commented Jul 24, 2018

@krvajal doesn't matter if it takes years. If someone else wants to help complete the PR, they're welcome to post a link to their commits here, and I'll pull them into the PR branch (please do not open a new PR).

@ephys
Copy link
Contributor Author

ephys commented Jul 25, 2018

I'm very sorry to be the source of the delay. I had completely forgotten about this PR.
I've updated my PR to add the necessary tests and I'll do my best to answer to future feedback in a timely fashion.

echenley
echenley reviewed Mar 1, 2019
View reviewed changes
src/core/importType.js
return !path || folders.some(folder => -1 < path.indexOf(join(folder, name)))

// extract the part before the first / (redux-saga/effects => redux-saga)
const packageName = name.match(/([^/]+)/)[0]
Copy link
Contributor

@echenley echenley Mar 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use const base = baseModule(name) instead, like in isBuiltIn? That appears to handle scoped packages.

@echenley
Copy link
Contributor

echenley commented Mar 1, 2019

@ephys This was approved a while back. Has this effort been abandoned?

@ljharb ljharb force-pushed the master branch 2 times, most recently from 3b453b4 to e9544f8 Compare March 3, 2019 07:03
@ljharb ljharb merged commit e9544f8 into import-js:master Mar 3, 2019
@outoftime outoftime mentioned this pull request Apr 5, 2019
Merged
@sprtvln sprtvln mentioned this pull request Jun 3, 2019
Closed
skozin added a commit to skozin/eslint-plugin-import that referenced this pull request Jan 11, 2020
@skozin
[Tests] enable a skipped test that's not failing anymore
f61c208 
This test was added and skipped in import-js#794 (probably since it was failing then), but it's not failing anymore on current master
ljharb pushed a commit to skozin/eslint-plugin-import that referenced this pull request Jan 12, 2020
@skozin @ljharb
[Tests] enable a skipped test that's not failing anymore
3568fea 
This test was added and skipped in import-js#794 (probably since it was failing then), but it's not failing anymore on current master
ljharb pushed a commit to skozin/eslint-plugin-import that referenced this pull request Jan 12, 2020
@skozin @ljharb
[Tests] enable a skipped test that's not failing anymore
d2fa2f4 
This test was added and skipped in import-js#794 (probably since it was failing then), but it's not failing anymore on current master
ljharb pushed a commit to skozin/eslint-plugin-import that referenced this pull request Jan 14, 2020
@skozin @ljharb
[Tests] enable a skipped test that's not failing anymore
6af5772 
This test was added and skipped in import-js#794 (probably since it was failing then), but it's not failing anymore on current master
ljharb pushed a commit to skozin/eslint-plugin-import that referenced this pull request Jan 14, 2020
@skozin @ljharb
[Tests] enable a skipped test that's not failing anymore
bbe456c 
This test was added and skipped in import-js#794 (probably since it was failing then), but it's not failing anymore on current master
@MaxMood96 MaxMood96 mentioned this pull request Feb 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb approved these changes

@jfmengels jfmengels Awaiting requested review from jfmengels

@benmosher benmosher Awaiting requested review from benmosher

+1 more reviewer

@echenley echenley echenley left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@benmosher benmosher

@jfmengels jfmengels

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

import/order fails after updating a dependency (redux-saga)