Refactor node module loading, and check circular dependencies

[jaszhix]

I ran into several issues when trying to require mathjs. After I rewrote the module loader so all of its dependencies could be found (and ran your tests and checked other libraries I've previously tried for regressions), I realized its just loading the modules endlessly. I went ahead and added a global that caches the module strings as passed to the load function in node_modules.js, and used that to check if there are circular dependencies. If so, an error is thrown.

There is a Babel plugin that might help: https://github.com/whitecolor/babel-plugin-cycle-circular

It would need to be compiled into a stand alone build so we could integrate it.

Other changes:

• Added a placeholder polyfill for a specific method that's used in the zlib (native) node module that Axios uses. Forgot I commented out its require during testing yesterday.

[WebReflection]

wow, huge change here 😄

I will double check this evening but please do the following:

npm install

and

make hint

just to keep the style consistent. Last PR I had to put few semi around and fix arrow functions too.

About arrow functions, please don't use them (yet) because in older Moz these are very buggy and I'd like to still support older GJS if possible, thanks.

[jaszhix]

Ah okay, sounds good. By the way, I pulled the newest Babel standalone and have modified it (tediously) so it doesn't give off mozjs' overly aggressive warnings. I know you disable them by default, but would you be interested in that PR?

[WebReflection]

Yes but maybe not here ? I'm using this module for the standalone, let's not mess up too much here 😃

https://github.com/WebReflection/babel-standalone

[jaszhix]

Oh wow, you already did it! Cool, well I have done what you did, but on a newer version. :)

[WebReflection]

it was in the package.json as dependency 😉

https://github.com/WebReflection/jsgtk/blob/master/package.json#L31

[jaszhix]

Yeah, but that's not useful if it breaks in GJS without modification. I always have to fix that set.forEach line.

Edit: Maybe its a difference with CJS? When you run it without the warnings flag it floods the terminal.

[WebReflection]

You have to use the No-Warning flag regardless because Mozilla chaps didn't go light at all with useless warnings, like the one about the proto and inevitable stuff after Babel.

I am not using CJS at all but I know jsgtk works there too so we might be carefull with changes there.

However, I think it's still about that repository, and not a PR that should land here.

[jaszhix]

Have you tested this on GJS 1.48? It has new warnings.

Edit: I guess we have different goals, but I'm only trying to PR things that are issues in either scenario. If you're not interested I can maintain my own patch set.

[WebReflection]

Yes, and already complained with the official ML.

Modern MozJS doesn't have those warnings, it's Mozillians overly warning for no real-world reason. Sometimes you just have to set the prototype to extend natives

[jaszhix]

Yeah, totally agree. Its the only way to compile ES2015 classes to ES5.

[WebReflection]

I'm going through these changes and there's a particular point that doesn't convince me: https://github.com/WebReflection/jsgtk/pull/9/commits/d5475d2c8065576fa2b13c27b1721a96576a3d4b#diff-c38256ee7429d502f9e9927cbc19898fR84

It looks like it's impossible to relatively include modules in the upper folder.

On top of that, every check for folders disappeared. Any particular reason to believe if you require a name it cannot be just a folder so that you can safely / easily check later on for index.js, package.json, etcetera?

Thanks for any clarification.

[jaszhix]

The relative paths are not being converted, that comment should have been removed, but it was left over from a block of logic I didn't leave in there that was splitting the string path into array, popping the last one, rejoining it. After I realized Gio was smart enough to understand the paths, I left it as is. The two checks on '.' and '..' are there because they interfered with the way the rest of the logic flow was handling the directories for either absolute paths or finding the package JSON file.

I did this a similar way in my npmatic project, which is a front end for npm via Electron. I put the top priority on finding the node modules folder the module belonged to, and hunting it down from there.

The folder check was removed in favor of it cycling through the formatting of the strings. If a file doesn't have an extension, its a folder usually. Unless someone requires an executable in a bin directory, but we could revert to using the folder checking method if you want.

Edit: String formatting line wasn't what I meant to refer to. In some cases I think the data being passed into the function is noise, because once you've found its module directory and read the package JSON file, you have all the info you need. If its absolute, it will default to index.js.

[WebReflection]

If a file doesn't have an extension, its a folder usually.

absolutely not. Create a test.js file and put the following inside:

module.exports = 'hello';

now node and then require('./test')

I appreciate your help here but I have the impression you think I didn't know what I was doing when I've written jsgtk (no setInterval and no setTimeout, random folders checks for no reasons on require).

Try to imagine I've been using JS for the last 18 years so that maybe if there are things around there is always a reason worth double checking before wiping them?

I think all you needed to do here was solving the infinite loop, you ended up breaking few working apps I have in here, dropping a bit too much.

TL;DR I cannot merge this as it is and I'm not sure it makes sense to push again, maybe I'll take some time to investigate how to solve your issue over what's working already?

Thanks for any sort of outcome.

[jaszhix]

Honestly not making any assumptions about you, but your code was broken, and mine works. That's a fact. I think I'm over your attitude, I have better things to do.

[WebReflection]

your code was broken, and mine works. That's a fact.

I've just written an example that breaks with your code and all I was saying is that it would be better for both of us to eventually file a bug with a test case that fails and after file a PR so that within the bug we can discuss before we both waste time.

However ...

I think I'm over your attitude

mine? I've just told you you broke the importer and there were reasons for checking folders.

I have better things to do.

Yes, Yes, me too 🎉

[jaszhix]

Try to imagine I've been using JS for the last 18 years

I have the impression you think I didn't know what I was doing

Yeah we're not comparing each other. The point was to fix the importer. The importer didn't work under normal use cases to begin with. Then every response you're scrutinizing me instead of maybe being appreciative someone even gives a crap enough to contribute? Yeah, I can't work under those conditions.

If you want me to see your point of view, you can start by not belittling me.

[WebReflection]

The importer didn't work under normal use cases to begin with.

it did for months in my projects

Then every response you're scrutinizing me

I just don't want to repeat what happened with last PR so I am suggesting how to contribute for consistency sake and future help from others.

instead of maybe being appreciative someone even gives a crap enough to contribute?

I've written thank you in more than an occasion but if you feel like you should be reworded for an Open Source project contribution, a project you need in the first place, then I don't want your contribution because this is not what Open Source is about and I rather stay away from people feeling entitled to write anything just because they are contributing.

Yeah, I can't work under those conditions.

That's ok but thanks for trying to help.

If you want me to see your point of view, you can start by not belittling me.

TIL a new word, thanks for that.

I don't want you to do anything, I'm telling you few things:

1. please keep the style consistent. I'm never difficult about code but there is a basic linter, please use it.
2. if you are removing a lot of lines of code from anything in here, consider it was probably there for a reason. Ask me and I'll explain so that neither you nor I would waste time reviewing broken releases
3. please squash PRs if you can, it'll be easy to review the whole thing and commenting it too
4. if you aren't sure about something (like the babel standalone or the setInterval and setTimeout) please ask before wasting time
5. please don't take code reviews personal, nobody is belittling you, I'm just code reviewing and commenting to help both of us next time.

If you cannot cope with these rules, feel free to fork and do your thing a part.

Thank you

[jaszhix]

Its not code reviewing when you're pointing out petty things like me forgetting something in your package.json. No your code does not work. Try npm install mathjs and requiring it. It will break because your switch case is not letting it get parsed the way it needs to.

I don't mind constructive criticism, but it seems you mistook my intention as an attempt to prove you're wrong on things, when I really just cared about fixing this. Never mind you didn't respond to my justification for the changes, but went into a rant about your qualifications being reason enough why you won't merge.

I also am not reproducing any errors when following your example (and apparently leading defense of not merging). You seem to be getting overly defensive about process instead of being pragmatic and just accepting a fix.

I've written thank you in more than an occasion but if you feel like you should be reworded for an Open Source project contribution, a project you need in the first place, then I don't want your contribution because this is not what Open Source is about and I rather stay away from people feeling entitled to write anything just because they are contributing.

I'm not entitling myself here. I'm simply raising awareness about bugs that you are now refusing to acknowledge as bugs. I'm not doing this for myself or my ego, and I don't care about being "rewarded" with a contribution. That doesn't make sense to me. My point was be nice to people, if you treat me like I have to work for you, then you're creating a toxic situation for a volunteer.

[WebReflection]

I'm simply raising awareness about bugs that you are now refusing to acknowledge as bugs.

can you point at me where exactly I've said there is no bug about recursive require?

If you can do that, we can continue to talk. Otherwise it looks like someone is overreacting all over the place and I do really have better things to do.

[jaszhix]

I think all you needed to do here was solving the infinite loop, you ended up breaking few working apps I have in here, dropping a bit too much

If you can do that, we can continue to talk.

No need, I'll maintain my own version.

[WebReflection]

P.S.

My point was be nice to people

imagine I arrive to your repository and start filing consistent PRs where I wipe out half of your file code, I introduce a redundant polyfill after me ignoring and commenting out random stuff and tell you how much work it was to have a babel standalone, the core of your very own project ...

Imagine that happens after 8 hours of work and last time you merged anyway you had to also fix linters issue and other warnings ...

Do you think you've been nice? I think you've tried to help and I've thanked you already but I needed to cut the mustard and tell you that impulsive PRs here are not so welcome, specially when these are redundant or breaking changes.

If we'd like to improve this project, both of us, we should find out what are the issue, file a bug with a number, provide a test case that breaks, and find the best solution.

You know, like any other Open Source project on this world I've ever worked on.

Take care

[jaszhix]

Do you think you've been nice?

Yes, because I would be patient enough to understand they were not doing any of it on purpose or intentional carelessness. Since I know nothing about this person, I'm not going to make any assumptions about him, or how he views my work.

Trust me, I'm sorry, this is clearly too much of a burden for you to test my PR. Getting defensive over process before testing a PR seems a bit out of touch. I suspect you didn't test it because you made up a fake bug this PR caused.

[WebReflection]

If we'd like to improve this project, both of us, we should find out what are the issue, file a bug with a number, provide a test case that breaks, and find the best solution.

[WebReflection]

This PR is not squashed. I cannot cherry pick easily this PR because it's split in many parts.

I have raised concerns about two things:

1. relative paths in the super folder
2. why there are no checks for folders yet

You haven't answered concretely to any of these doubts, you instead wrote:

The relative paths are not being converted, that comment should have been removed

then where's the update?

Unless someone requires an executable in a bin directory, but we could revert to using the folder checking method if you want.

should we? where is the change?

Edit: String formatting line wasn't what I meant to refer to. In some cases I think the data being passed into the function is noise

why don't we fix the noise?

etcetera

[jaszhix]

Because I was busy, and I'm still doing other things. You made up your mind about not merging before I had a chance to look into it.

[WebReflection]

You even deleted already the branch and my "not merging" had a question mark at the end, it was a question because of the many changes without squashes.

I still would like to have a test case to validate against but I'll fix this at some point anyway.

Take care, this is my last reply for today.

[jaszhix]

No..

TL;DR I cannot merge this as it is and I'm not sure it makes sense to push again, maybe I'll take some time to investigate how to solve your issue over what's working already?

You were posing whether you fix it as the question, not if you should merge.

[WebReflection]

I cannot merge this as it is and I'm not sure it makes sense to push again

[jaszhix]

Which boils down to fuck off, I don't want your help.

[WebReflection]

that's your attitude problem, really, but good point:

6. please avoid any insult, direct or indirect, to anyone in this repository

This is a first, and last warning, before blocking you.

If you came here to have a fight and put words or thoughts in my mouth/head I'm not here for any show.

I suggest you just stop replying and have some rest. I need to go

[jaszhix]

It was meant to be paraphrasing as an example, not trying to curse at you dude. Chill out.