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

Fix browserify issue #101

Merged
merged 4 commits into from
Oct 14, 2015
Merged

Conversation

BenjaminVanRyseghem
Copy link
Owner

Fix #70
Fix #71
Fix #79

Fix #71
Fix #79
var langFiles = fs.readdirSync(path.join(__dirname, 'languages'));
langFiles.forEach(function (langFile) {
numbro.language(path.basename(langFile, '.js'), require(path.join(__dirname, 'languages', langFile)));
});
Copy link
Contributor

Choose a reason for hiding this comment

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

You implied that you did not want to break #62 or revert #66 in our conversation in #79. By removing this code, doesn't that break #62/#66?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I put back the automatic load 😄

@clayzermk1
Copy link
Contributor

@stewart42, @alexkwolfe care to weigh in on this patch with respect to #62/#66?

@BenjaminVanRyseghem
Copy link
Owner Author

@clayzermk1 I wanted to test first that the new inNodejsRuntime works correctly.

If it does, we can put it back here

@BenjaminVanRyseghem
Copy link
Owner Author

but maybe having it as a separated functions as you suggested is a good idea 😄

}

function inNodejsRuntime() {
return process.browser === undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure, but I don't think this is universal for client-side CommonJS implementations. It should work in browserify, but no clue about webpack or others. I don't think it is standardized. process.title === 'node' seems to be more standard from what I could find.

Copy link
Owner Author

Choose a reason for hiding this comment

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

ok 😄

It's what I found after some googling.
maybe use both ?

}

function inNodejsRuntime() {
return process && process.browser === undefined && (process.title === 'node' || process.title === 'grunt');
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an issue with using this line and browserify, process is not defined by default with browserify.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a typeof process !== 'undefined' at the beginning? Possibly instead of the first process in the condition?

Replacing the first process with typeof process !== 'undefined' worked for me.

Copy link
Owner Author

Choose a reason for hiding this comment

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

if process is undefined, inNodejsRuntime returns false, or?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, throws a "process is not defined" error.

Copy link
Owner Author

Choose a reason for hiding this comment

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

weird 😄

I will fix it then 😉

@BenjaminVanRyseghem
Copy link
Owner Author

is it a merge candidate now?

@clayzermk1
Copy link
Contributor

LGTM for my purposes, no errors during a quick test.

BenjaminVanRyseghem added a commit that referenced this pull request Oct 14, 2015
@BenjaminVanRyseghem BenjaminVanRyseghem merged commit 7d16321 into develop Oct 14, 2015
@BenjaminVanRyseghem BenjaminVanRyseghem deleted the try_to_fix_browserify_issue branch October 14, 2015 08:27
@BenjaminVanRyseghem
Copy link
Owner Author

done then ;)

@BenjaminVanRyseghem
Copy link
Owner Author

in numbro 1.5.2

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

Successfully merging this pull request may close these issues.

None yet

3 participants