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

Add find method to require #70

Closed
rumkin opened this issue Dec 4, 2014 · 10 comments
Closed

Add find method to require #70

rumkin opened this issue Dec 4, 2014 · 10 comments

Comments

@rumkin
Copy link
Contributor

rumkin commented Dec 4, 2014

Actually it's very strange behaviour of require.resolve, which throws an error when package not found. Add require.find method which would return null if package not found.

@rumkin rumkin changed the title Add find method to require Add find method to require Dec 4, 2014
@missinglink
Copy link
Contributor

Implementing this behaviour on your own is trivial:

function find( moduleName ){
  try {
    return require.resolve( moduleName );
  } catch( e ){
    return null;
  }
}

What exactly is strange about it and why do you feel it is a wide enough issue that it deserves a new core API method?

@rumkin
Copy link
Contributor Author

rumkin commented Dec 4, 2014

@missinglink Yep, it's trivial. And platform API should be clean and trivial, isn't? But the problem is that require is context-dependent function (it works different in each file). So it means that I should to implement find method in each file. It's not rational. And finally if there is way to do something without throwing an Error it should be done so.

@fampinheiro
Copy link

@rumkin you can use a module like optional if you don't want to wrap your requires.

@feross
Copy link
Contributor

feross commented Dec 4, 2014

-1. The module loading code is Stability: 5 - Locked and unlikely to ever change.

Just use @missinglink's snippet or the optional module.

@rumkin
Copy link
Contributor Author

rumkin commented Dec 4, 2014

@fampinheiro Are you serious? Was you looking it's source? It will not work like it should because it not context-dependent and couldn't be.
@feross It's for node API. iojs will change loading code because it cannot use node_modules folder forever.

@aredridel
Copy link
Contributor

Why can't it?

@rumkin
Copy link
Contributor Author

rumkin commented Dec 4, 2014

@aredridel Because iojs could not control Joyent policy so backward compatibility will be broken soon (after version 1.0). To prevent a mess of io and node packages module directory should be renamed.

@aredridel
Copy link
Contributor

Not sure why they'd be separate. node_modules is deeply baked in, and the module system is locked stability.

and there's no guarantee that a breaking change will ever happen, nor would changing the folder name help that (it'd only break more)

@missinglink
Copy link
Contributor

@rumkin, @aredridel please stick to the reported issue and avoid speculation.
https://github.com/iojs/io.js/blob/v0.12/CONTRIBUTING.md#issue-contributions

@chrisdickinson
Copy link
Contributor

I don't think this will change in the near future:

  1. It would expand the surface area of a locked subsystem.
  2. This behavior can be approximated in userland (and there exist packages to make that easier!)

Closing for now.

kunalspathak added a commit to kunalspathak/node that referenced this issue Feb 26, 2017
1. We parse error stack in chakra_shim to make it simliar to that
of v8. However we were missing the case if stack messages has
`\n`. Fixed it.
Fixes: nodejs#78

2. Fixed Error.captureStackTrace

Since `chakra_shim.js` that patches `Error.captureStackTrace` runs
under strict mode, it was not able to get caller information. Having
caller information is much reliable in captureStackTrace than
matching using `function.name`. Removed `use strict` and disabled
eslint rule for `use strict`.
This helped in matching caller exactly regardless of name of form
`o.p.q` in `Error.captureStackTrace`.

Fixes: nodejs#75

3. Added missing functions on StackFrame

Added following missing methods on StackFrame:
* getFunction
* getMethodName
* getTypeName
* isConstructor
* isToplevel
* isNative

Implemented `getFunction`.

`getFunction` currently works if `.stack` is called from same
callsite where `new Error()` or `Error.captureStackTrace()` was
called. However currently we don't record and store the callstack
when these functions were called and so we lose the information
of `.caller` when called later while populating `e.stack`.

If we save the callstack, we would be holding references to all the
functions in the callstack which doesn't seem right.

I have added a TODO to solve this, but wanted to send this out
so we get better coverage on node compatibility.

Fixes : nodejs#70

PR-URL: nodejs/node-chakracore#85
Reviewed-By: Jianchun Xu <[email protected]>
boingoing added a commit to boingoing/node that referenced this issue Apr 6, 2017
First, GCC doesn't seem to implement an interface keyword. Workaround here is to use struct instead of interface. This is nodejs/abi-stable-node#69

Second, GCC doesn't support the flavor of static_assert which takes a single parameter. Just added a second parameter message here.
targos added a commit to targos/node that referenced this issue Apr 17, 2021
Original commit message:

    [M86 LTS] Disable failing tests

    Disable failing tests backported from ToT. No existing tests
    are disabled.

    No-Try: true
    No-Presubmit: true
    No-Tree-Checks: true
    Bug: None
    Change-Id: I94d2cd4827ce6fd1875c66912b4841a4a7c72ab3
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2764754
    Reviewed-by: Artem Sumaneev <[email protected]>
    Commit-Queue: Victor-Gabriel Savu <[email protected]>
    Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#70}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@5678ebe
targos added a commit to targos/node that referenced this issue Apr 27, 2021
Original commit message:

    [M86 LTS] Disable failing tests

    Disable failing tests backported from ToT. No existing tests
    are disabled.

    No-Try: true
    No-Presubmit: true
    No-Tree-Checks: true
    Bug: None
    Change-Id: I94d2cd4827ce6fd1875c66912b4841a4a7c72ab3
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2764754
    Reviewed-by: Artem Sumaneev <[email protected]>
    Commit-Queue: Victor-Gabriel Savu <[email protected]>
    Cr-Commit-Position: refs/branch-heads/8.6@{nodejs#70}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@5678ebe
targos added a commit that referenced this issue Apr 30, 2021
Original commit message:

    [M86 LTS] Disable failing tests

    Disable failing tests backported from ToT. No existing tests
    are disabled.

    No-Try: true
    No-Presubmit: true
    No-Tree-Checks: true
    Bug: None
    Change-Id: I94d2cd4827ce6fd1875c66912b4841a4a7c72ab3
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2764754
    Reviewed-by: Artem Sumaneev <[email protected]>
    Commit-Queue: Victor-Gabriel Savu <[email protected]>
    Cr-Commit-Position: refs/branch-heads/8.6@{#70}
    Cr-Branched-From: a64aed2333abf49e494d2a5ce24bbd14fff19f60-refs/heads/8.6.395@{#1}
    Cr-Branched-From: a626bc036236c9bf92ac7b87dc40c9e538b087e3-refs/heads/master@{#69472}

Refs: v8/v8@5678ebe

PR-URL: #38275
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
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

No branches or pull requests

6 participants