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

consider modules from bower_components' and 'jspm_modules as external modules #444

Merged
merged 4 commits into from
Jul 21, 2016
Merged

consider modules from bower_components' and 'jspm_modules as external modules #444

merged 4 commits into from
Jul 21, 2016

Conversation

zloirock
Copy link
Contributor

In one project we wanna use order rule with configuration

{
    "groups": [
        ["builtin", "external"],
        ["internal", "parent", "sibling", "index"]
    ],
    "newlines-between": "always"
}

but our frontend uses bower and webpack aliases.

@coveralls
Copy link

coveralls commented Jul 19, 2016

Coverage Status

Coverage remained the same at 97.615% when pulling ae1fc37 on zloirock:external-modules-fix into b2184f0 on benmosher:master.

@benmosher
Copy link
Member

benmosher commented Jul 19, 2016

Interesting, makes sense to me.

What about using a setting (how does import/external-module-folders sound?)--like import/core-modules for the builtin designation--and defaulting to what you've got defined for folders currently?

Then the next person who wants to add rollup_modules_2017 or whatever is already covered.

Also, docs, tests, and changelog notes please! 😁

@zloirock
Copy link
Contributor Author

What about using a setting (how does import/external-module-folders sound?)

Yep, makes sense, I thought about it.

Also, docs, tests, and changelog notes please!

Sure, I'll add it, the first commit was mainly checking you interest in this feature :)

@coveralls
Copy link

coveralls commented Jul 20, 2016

Coverage Status

Coverage remained the same at 97.628% when pulling 36b9ccb on zloirock:external-modules-fix into 7990596 on benmosher:master.

@coveralls
Copy link

coveralls commented Jul 20, 2016

Coverage Status

Coverage remained the same at 97.628% when pulling 36b9ccb on zloirock:external-modules-fix into 7990596 on benmosher:master.

it("should return 'external' for module from 'node_modules' if 'node_modules' contained in 'external-module-folders'", function() {
const foldersContext = testContext({ 'import/external-module-folders': ['node_modules'] })
expect(importType('builtin-modules', foldersContext)).to.equal('external')
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a mock for resolve, so I used the first module from dependencies for testing.

@zloirock
Copy link
Contributor Author

Updated.

@coveralls
Copy link

coveralls commented Jul 20, 2016

Coverage Status

Coverage increased (+0.01%) to 97.643% when pulling 38c28f1 on zloirock:external-modules-fix into 7990596 on benmosher:master.

@benmosher
Copy link
Member

Looks great! Thanks!

@benmosher benmosher merged commit 21798a8 into import-js:master Jul 21, 2016
@zloirock zloirock deleted the external-modules-fix branch July 26, 2016 08:37
@zloirock
Copy link
Contributor Author

@benmosher can you publish a new release?

@benmosher
Copy link
Member

benmosher commented Jul 26, 2016

Just published as v1.12.0! 😎 Thanks again!

@zloirock
Copy link
Contributor Author

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants