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

Feature/internal resolve #4315

Merged
merged 20 commits into from
Aug 22, 2017
Merged

Conversation

tvald
Copy link
Contributor

@tvald tvald commented Aug 21, 2017

Summary
Removes dependency on resolve package and pulls all necessary code into default_resolver.js, with minor cleanup and added type definitions.

This is a preparatory step for improving the performance of resolve (#2925).

Test plan

Passes all existing tests of the legacy resolve.sync() behavior.

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@tvald
Copy link
Contributor Author

tvald commented Aug 21, 2017

Not sure about the above errors, but I don't think they're related to this PR. Locally, all tests pass when I yarn test. (except four related to hg, which I don't have installed)

@tvald
Copy link
Contributor Author

tvald commented Aug 21, 2017

Fixed one problem that wasn't locally checked. (jest-resolve/index.js accessed the internals of the resolve package to leverage the helper node_modules_paths, which is now exposed within jest-resolve)

@tvald
Copy link
Contributor Author

tvald commented Aug 21, 2017

Note that there is a new file - jest-resolve/node_modules_paths.js, which should have the standard ++header++ added to it. (I don't know what your current standards are, so I'll leave that to a maintainer.)

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@@ -0,0 +1,47 @@
import type {Path} from 'types/Config';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add the copyright header just like in other files?
Also, since it's adapted from resolve, an extra annotation is necessary:

 /**
  * Copyright (c) 2014, Facebook, Inc. All rights reserved.
  *
  * This source code is licensed under the BSD-style license found in the
  * LICENSE file in the root directory of this source tree. An additional grant
  * of patent rights can be found in the PATENTS file in the same directory.
  *
  * Adapted from: https://github.com/substack/node-resolve
  * 
  * @flow
  */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 3313380

@tvald
Copy link
Contributor Author

tvald commented Aug 22, 2017

One more fix, this time to remove unused optional fields that flow(?) was complaining about.

Generally speaking, it would be nice if (a) CI didn't fail so frequently for unrelated problems, and (b) it was possible / obvious how to run all checks locally.

@cpojer
Copy link
Member

cpojer commented Aug 22, 2017

Thanks for the initial PR, this is great! In a follow-up, do you mind changing all the single-expression if-statements to wrap them with curly-braces? That's the code style we use in this repo :)

@cpojer
Copy link
Member

cpojer commented Aug 22, 2017

Also, could we add support for the "browser" field in a follow-up and remove "browser-resolve"? I would love it if we could drop that dependency :)

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants