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

Expose hook into the module resolution (resolve) #153

Merged
merged 1 commit into from
Sep 8, 2020
Merged

Expose hook into the module resolution (resolve) #153

merged 1 commit into from
Sep 8, 2020

Conversation

sergioramos
Copy link
Contributor

No description provided.

@sergioramos sergioramos marked this pull request as draft August 25, 2020 22:27
src/types.ts Outdated Show resolved Hide resolved
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Tests are failing

@sergioramos sergioramos marked this pull request as ready for review August 31, 2020 18:32
@sergioramos
Copy link
Contributor Author

@styfle tests passing now 😉

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

This approach looks great, thanks for implementing it.

src/resolve-dependency.ts Outdated Show resolved Hide resolved
src/node-file-trace.ts Show resolved Hide resolved
test/fixtures/yarn-pnp/package.json Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@sergioramos
Copy link
Contributor Author

Implemented the suggestions and got the tests passing again 😉

src/node-file-trace.ts Outdated Show resolved Hide resolved
test/unit.test.js Outdated Show resolved Hide resolved
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Great work, thanks!

@styfle styfle added the automerge Automatically merge PR once checks pass label Sep 8, 2020
@kodiakhq kodiakhq bot merged commit f454be5 into vercel:master Sep 8, 2020
@sergioramos sergioramos deleted the resolve branch September 8, 2020 22:15
ramosbugs added a commit to ramosbugs/serverless-next.js that referenced this pull request Nov 7, 2020
This PR makes two changes to the `lambda-at-edge` builder to better
support Yarn v2 projects when using the `serverless-trace` target.

1. Adds a `baseDir` build option to specify the base directory to search
   for `node_modules`. Currently, the builder sets this to `process.cwd()`,
   but Yarn v2 often hoists dependencies across multiple workspaces so
   that they can be shared. Without this change, all dependencies from
   ancestor directories are omitted, leading to import errors at runtime.
2. Adds a `resolve` build option which allows projects to specify their
   own custom resolvers, such as when supporting Yarn v2's PnP mode.
   This leverages the resolver hook added in vercel/nft#153, which also
   required upgrading from `@zeit/node-file-trace` to the latest version
   of `@vercel/nft`. Note that even after this change, Yarn v2's PnP mode
   is still incompatible with the builder since the builder ends up
   collapsing all of the PnP dependencies into a single `node_modules`
   directory. This causes multiple versions of a package to clobber one
   another.
ramosbugs added a commit to ramosbugs/serverless-next.js that referenced this pull request Nov 7, 2020
This PR makes two changes to the `lambda-at-edge` builder to better
support Yarn v2 projects when using the `serverless-trace` target.

1. Adds a `baseDir` build option to specify the base directory to search
   for `node_modules`. Currently, the builder sets this to `process.cwd()`,
   but Yarn v2 often hoists dependencies across multiple workspaces so
   that they can be shared. Without this change, all dependencies from
   ancestor directories are omitted, leading to import errors at runtime.
2. Adds a `resolve` build option which allows projects to specify their
   own custom resolvers, such as when supporting Yarn v2's PnP mode.
   This leverages the resolver hook added in vercel/nft#153, which also
   required upgrading from `@zeit/node-file-trace` to the latest version
   of `@vercel/nft`. Note that even after this change, Yarn v2's PnP mode
   is still incompatible with the builder since the builder ends up
   collapsing all of the PnP dependencies into a single `node_modules`
   directory. This causes multiple versions of a package to clobber one
   another.
danielcondemarin added a commit to serverless-nextjs/serverless-next.js that referenced this pull request Nov 11, 2020
…less-trace (#779)

* Add baseDir and resolve build options to serverless-trace

This PR makes two changes to the `lambda-at-edge` builder to better
support Yarn v2 projects when using the `serverless-trace` target.

1. Adds a `baseDir` build option to specify the base directory to search
   for `node_modules`. Currently, the builder sets this to `process.cwd()`,
   but Yarn v2 often hoists dependencies across multiple workspaces so
   that they can be shared. Without this change, all dependencies from
   ancestor directories are omitted, leading to import errors at runtime.
2. Adds a `resolve` build option which allows projects to specify their
   own custom resolvers, such as when supporting Yarn v2's PnP mode.
   This leverages the resolver hook added in vercel/nft#153, which also
   required upgrading from `@zeit/node-file-trace` to the latest version
   of `@vercel/nft`. Note that even after this change, Yarn v2's PnP mode
   is still incompatible with the builder since the builder ends up
   collapsing all of the PnP dependencies into a single `node_modules`
   directory. This causes multiple versions of a package to clobber one
   another.

Co-authored-by: Daniel <danielconde9@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Automatically merge PR once checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants