Skip to content

Ensure _loaderHelpers registers actual filenames#24

Merged
hueniverse merged 1 commit intohapijs:masterfrom
keithamus:patch-1
Jun 21, 2015
Merged

Ensure _loaderHelpers registers actual filenames#24
hueniverse merged 1 commit intohapijs:masterfrom
keithamus:patch-1

Conversation

@keithamus
Copy link
Copy Markdown
Contributor

Slicing a path by the last 3 characters works for two character extensions, e.g. '.js' or '.cs' but not for longer extensions ('.es6' for example).

Using Path.extname will extract the proper extension name from a path, enabling support for files with long extensions.

@jagoda
Copy link
Copy Markdown
Contributor

jagoda commented Apr 14, 2015

I don't believe that Path.parse() is available in Node 0.10.

@keithamus
Copy link
Copy Markdown
Contributor Author

@jagoda, you're totally right, my bad. I noticed this as I disappointedly watched travis fail. I've updated with code that works in .10, and also some tests to ensure the desired functionality actually works. (Also updated OP to reflect actual change)

lib/manager.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think that this will work correctly for helpers that are in subdirectories. Any reason to not use the original approach but just replace -3 with -Path.extname(file).length?

Slicing a path by the last 3 characters works for two character
extensions, e.g. `'.js'` or `'.cs'` but not for longer extensions
(`'.es6'`, `'.javascript'`, `'.coffeescript'` for example).

Using `Path.extname` will extract the proper extension from a path,
which can then be used to determine the length of the extension proper,
and thereby allowing helpers with longer extension names.
@keithamus
Copy link
Copy Markdown
Contributor Author

@jagoda amended.

@jagoda
Copy link
Copy Markdown
Contributor

jagoda commented Apr 15, 2015

Cool. Looks good to me.

@keithamus
Copy link
Copy Markdown
Contributor Author

@hueniverse I can has merge?

@jagoda
Copy link
Copy Markdown
Contributor

jagoda commented May 21, 2015

@hueniverse I am happy to merge this if given the commit bit.

@hueniverse hueniverse added the bug Bug or defect label Jun 21, 2015
@hueniverse hueniverse added this to the 2.0.1 milestone Jun 21, 2015
@hueniverse hueniverse self-assigned this Jun 21, 2015
hueniverse pushed a commit that referenced this pull request Jun 21, 2015
Ensure _loaderHelpers registers actual filenames
@hueniverse hueniverse merged commit 3e085c4 into hapijs:master Jun 21, 2015
@keithamus keithamus deleted the patch-1 branch June 22, 2015 10:25
@lock
Copy link
Copy Markdown

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug Bug or defect

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants