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

Support workspaces #1500

Closed
wants to merge 1 commit into from
Closed

Conversation

bradleyayers
Copy link

The location of node_modules is not guaranteed to be relative to the current directory (e.g. in yarn workspaces).

A better solution here is to update package.json exports to include the file.

Description

Breaking change?

Example


Checklist

  • Tests added in this PR (if applicable)

The location of `node_modules` is not guaranteed to be relative to the current directory (e.g. in yarn workspaces). 

A better solution here is to update package.json `exports` to include the file.
@CLAassistant
Copy link

CLAassistant commented Jul 9, 2024

CLA assistant check
All committers have signed the CLA.

@@ -1,5 +1,6 @@
import yargs from "yargs";
import { hideBin } from "yargs/helpers";
import { join } from "node:path";
Copy link
Collaborator

@hugocaillard hugocaillard Jul 9, 2024

Choose a reason for hiding this comment

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

Looks like it's not being used

@hugocaillard
Copy link
Collaborator

hugocaillard commented Jul 9, 2024

Good catch, thank you for opening this PR @bradleyayers
I'll give it a try and need to make sure it doesn't have unwanted side effects

@bradleyayers
Copy link
Author

One thing to check is whether require.resolve is the correct API to use for ESM output, i'm not sure. And also the point about instead of doing a manual path concat i think it's better if the target file is made one of the exports in package.json.

@hugocaillard hugocaillard self-assigned this Jul 15, 2024
Copy link
Collaborator

@hugocaillard hugocaillard left a comment

Choose a reason for hiding this comment

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

remove unused imports

@hugocaillard
Copy link
Collaborator

Hey @bradleyayers
Sorry we couldn't get this merged before, now there is conflict. The file has been moved, you basically need to migrate the your change in this file: components/clarinet-sdk/node/src/vitest/index.ts.

Do you think you'll have time to do it or should I take over the task (and credit you in my commit)?

@bradleyayers
Copy link
Author

Please take this over that would be amazing, thank you.

@hugocaillard
Copy link
Collaborator

Superseded by #1514

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants