search

aseemk / requireDir

Node.js helper to require() directories.
MIT License
484 stars 60 forks source link

"Cannot read property 'filename' of undefined" error when using "--experimental-modules" flag #61

Open jvmunhoz opened 5 years ago

jvmunhoz commented 5 years ago

Screenshot_1

I'm using Windows 10 Pro, npm version 6.9.0, node 12.4.0 and require-dir 1.2.0

As soon as I go back to using require("") without the --experimental-modules flag it works again.

I'm relatively new to javascript, so it may be a rookie mistake by me 😝

yocontra commented 4 years ago

Can you post the code to reproduce this?

Yimura commented 3 years ago

He's using ES Module syntax, in ES Module the parent variable is undefined. That's causing this error.

Edit: Just noticed this issue is a year old, you can probably just close it then.

yocontra commented 3 years ago

@Yimura I don't think it should be closed since it should still be an issue - this module is failing if parent is not defined, which seems to be the case with the new node modules stuff.

Yimura commented 3 years ago

I wrote the following npm package and it does the same as this module (with a bit less functionality), the difference being that it supports the new ES Module style of importing modules.

Link to repo: https://github.com/Yimura/importDir

yocontra commented 3 years ago

@Yimura A PR would have worked too, no need to create a whole new project. This one is actively maintained by me.

The implementation plan to solve this ticket is pretty straightforward - make the relative path behavior optional, and document it - if you use ESM, pass an absolute path.

There are also other less conventional ways to get the parent path to keep the same behavior with ESM, new Error().stack can be used to easily do this.

ghost commented 3 years ago

This one is actively maintained by me.

Is it though? I, personally, wouldn't call something which had its last commit in 2018 and also 5 issues + 1 PR open "actively maintained". But that might just be me.

Yimura commented 3 years ago

I'm personnaly against merging CommonJS and ESModule functionality. And there are some project constraints as well.

  1. The package.json needs a key value combo of "type": "module", this will cause conflicts with import and require statements when launching projects.
  2. People would prefer keeping package sizes down, no need for redundant code that isn't used in the running env.

However I thank you for your hacky method to get the relative path through Error().stack, it was quite clever and I hadn't thought of it before. As a side note I prefer refraining from using hacky methods as ESModule has just been taken out of experimental launch flags (.json isn't even supported yet) so this feature is possibly being implemented (through experimental launch flags as well).

yocontra commented 3 years ago

@Bara-dev I'm not the original author of this module - I stepped in to maintain it and be responsive to PRs + answer issues and keep it up to date. I read + respond to every issue within 24hrs. All open issues are non-urgent feature requests aside from this one, and the one open PR was abandoned by the person who submitted it prior to me coming in to maintain this project. They haven't responded to pings since I got added in 2018, so I don't think it is fair to call something abandoned because it has one open PR. No commits or releases since 2018 means everything is working and doesn't need to be changed - I'm not going to be pushing breaking changes for the sake of activity.

@Yimura Yeah, using the Error() constructor in that way is a hack - I would also prefer not to do it that way. I'm not trying to encourage hostility I just think it would have been better to join efforts on a solution instead of forking and not contributing back. This isn't even my project, so I really don't have any ego about other packages existing to fill the same need - I maintain this purely because I use it in my own work and need it to work. Feel free to fork + use your own package however you want if you feel that is the best approach for you.

Yimura commented 3 years ago

The issue I brought up by having to set the type to module in the package.json file will make it so CommonJS can not run under the same project unless from a dependency. But that wouldn't make it part of this package...

I'm still giving credit to this package as well does the repo show that it is forked. I just don't see a way to merge both of these into one package.