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

fix: don't preserve symlinks so we can resolve npm deps in the symlinked node_modules tree #57

Merged
merged 1 commit into from
Aug 9, 2022

Conversation

gregmagolan
Copy link
Member

@gregmagolan gregmagolan commented Aug 9, 2022

Don't preserve symlinks since doing so breaks node_modules resolution in the pnpm-style symlinked node_modules structure. See https://pnpm.io/symlinked-node-modules-structure.

NB: esbuild will currently leave the sandbox and end up in the output tree until symlink guards are created to prevent this. See #32.

@gregmagolan gregmagolan requested a review from mattem August 9, 2022 00:27
@gregmagolan gregmagolan force-pushed the dont_do_the_thing_that_breaks_things branch 2 times, most recently from c3cfecc to 0909898 Compare August 9, 2022 00:45
examples/banner/BUILD.bazel Outdated Show resolved Hide resolved
@matthewjh
Copy link

matthewjh commented Aug 9, 2022

@gregmagolan can you also copy the srcs to a directory and then run esbuild on the files in the directory? When using rules_esbuild, plus preserveSymlinks = False, with rules_ts, esbuild will prefer the .ts files copied into the bindir by rules_ts over .js files intended as inputs, after it follows the symlinks into the bindir.

I had weird bugs from this as esbuild then performs its own typescript transpilation, potentially under a different config depending on which tsconfig, if any, it picks up. My solution was to copy the esbuild srcs to a dir, but I think most people would need this. Of course, this would be removed once the guards are in place.

@gregmagolan gregmagolan force-pushed the dont_do_the_thing_that_breaks_things branch 2 times, most recently from 873ef56 to 97cd74f Compare August 9, 2022 15:52
@gregmagolan gregmagolan force-pushed the dont_do_the_thing_that_breaks_things branch from 97cd74f to d6ef450 Compare August 9, 2022 16:01
@gregmagolan
Copy link
Member Author

gregmagolan commented Aug 9, 2022

@gregmagolan can you also copy the srcs to a directory and then run esbuild on the files in the directory? When using rules_esbuild, plus preserveSymlinks = False, with rules_ts, esbuild will prefer the .ts files copied into the bindir by rules_ts over .js files intended as inputs, after it follows the symlinks into the bindir.

I had weird bugs from this as esbuild then performs its own typescript transpilation, potentially under a different config depending on which tsconfig, if any, it picks up. My solution was to copy the esbuild srcs to a dir, but I think most people would need this. Of course, this would be removed once the guards are in place.

Right. I recall @mattem mentioning something like this in the past. Seems like something we could add in the interim until the guards are available. Thanks for the suggestion.

@mattem
Copy link
Member

mattem commented Aug 9, 2022

Yeah I've been setting this when I've been using rules_esbuild

config = {
        "resolveExtensions": [".js"],
    },

Copy link
Member

@mattem mattem left a comment

Choose a reason for hiding this comment

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

🌮 for removing cat too 😸

@gregmagolan gregmagolan merged commit fe714f6 into main Aug 9, 2022
@matthewjh
Copy link

Thanks @gregmagolan @mattem

@@ -203,7 +203,13 @@ def _esbuild_impl(ctx):
"logLimit": 0,
"metafile": ctx.attr.metafile,
"platform": ctx.attr.platform,
"preserveSymlinks": True,
# Don't preserve symlinks since doing so breaks node_modules resolution
# in the pnpm-style symlinked node_modules structure.

Choose a reason for hiding this comment

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

What precisely breaks? It's not obvious from the pnpm documentation linked here.

Copy link
Member Author

@gregmagolan gregmagolan Jan 15, 2023

Choose a reason for hiding this comment

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

Transitive deps of npm packages cannot be resolved without following symlinks in the symlinks node_modules structure: https://pnpm.io/symlinked-node-modules-structure. You'll resolve direct deps but any deps of deps that are also not also direct deps won't be found.

Choose a reason for hiding this comment

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

Also, the same package resolved through different symlinks will be treated
as a multiple distinct packages, which is wrong

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

Successfully merging this pull request may close these issues.

5 participants