Skip to content
This repository has been archived by the owner on Jan 25, 2020. It is now read-only.

Bug: Fix bad duplicate name resolution #78

Merged
merged 2 commits into from
Jan 15, 2016
Merged

Conversation

jasisk
Copy link
Member

@jasisk jasisk commented Jan 13, 2016

We were using require.resolve to ensure we used the built-in resolution mechanism however this is insufficient.

require.resolve will resolve if given a path with extension but, without one, it attepts to resolve with the registered extensions. That's why we did it. Problem is, if you have two files with the same name but different extensions, both will pass the resolution test if at least one of them uses a registered extension.

This change supports the existing resolution process in node (treat extension-less files as *.js, check files with extensions against the registered handlers). It will also prevent errors from spawning when a file fails to load (though it will still fail loudly if the loaded file doesn't have a signature of function (thing) {}.

Closes #77

@aredridel
Copy link

Will this mess up coffeescript users?

@jasisk
Copy link
Member Author

jasisk commented Jan 14, 2016

Will this mess up coffeescript users?

Nope. Checks against registered require extensions just the same (see the custom extension test). I don't like recreating core resolution functionality but c'est la vie.

@aredridel
Copy link

Makes sense. LGTM then!

@totherik
Copy link
Member

👍

@xjamundx
Copy link

I have no rights on this repo, but I have verified the issue and it addresses the problem I reported in #77

@jasisk
Copy link
Member Author

jasisk commented Jan 15, 2016

Publishing this as a patch. It may cause some problems but it will be with folks relying on behavior that is counter the documentation.

jasisk added a commit that referenced this pull request Jan 15, 2016
Bug: Fix bad duplicate name resolution
@jasisk jasisk merged commit 6bce76b into v1.2.x-release Jan 15, 2016
@jasisk jasisk deleted the fix-build-artifacts branch January 15, 2016 22:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants