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.

Sign up for GitHub

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

Jump to bottom

Remove nx dependency #560

Closed
bmingles opened this issue Jun 17, 2024 · 3 comments · Fixed by #561 or #562
Closed

Remove nx dependency #560

bmingles opened this issue Jun 17, 2024 · 3 comments · Fixed by #561 or #562
Assignees
@bmingles
Labels
enhancement New feature or request triage

Comments

@bmingles
Copy link
Contributor

We currently have useNx enabled for lerna in the plugins repo. Unfortunately nx has optional architecture specific dependencies that intermittently cause us issues since npm doesn't always install optional dependencies of dependencies.

One option to fix this is to hoist the optional dependencies to the root (tried in rejected PR #471)

The other / preferable option would be to just remove the nx dependency altogether.
@bmingles bmingles added enhancement New feature or request triage labels Jun 17, 2024
@bmingles bmingles self-assigned this Jun 17, 2024
bmingles added a commit to bmingles/deephaven-plugins that referenced this issue Jun 17, 2024
@bmingles
Remove nx dependency (deephaven#560)
2c0da22
@bmingles bmingles mentioned this issue Jun 17, 2024
Merged
@mattrunyon
Copy link
Collaborator

I don't think we can remove the dependency entirely. It's required for lerna and lerna is what we use for running tasks.

We can disable or run w/ NX_NON_NATIVE_HASHER=true. It seems that platform specific package includes a Rust hasher instead of using git for hashing file changes

@bmingles
Copy link
Contributor Author

We don't have a dependency on nx in web-client-ui. Removing seems to work fine

@mattrunyon
Copy link
Collaborator

lerna depends on it, but lists a range. So we have it pinned in our package.json for consistency

Removing it from our package.json just makes it an implicit dependency that we can't control the version of

bmingles added a commit to bmingles/deephaven-plugins that referenced this issue Jun 17, 2024
@bmingles
Restored nx version pinning (deephaven#560)
30fb966
bmingles added a commit to bmingles/deephaven-plugins that referenced this issue Jun 17, 2024
@bmingles
Hoisting @rollup optionalDependencies (deephaven#560)
c9824d5
bmingles added a commit to bmingles/deephaven-plugins that referenced this issue Jun 17, 2024
@bmingles
Hoisting rollup and esbuild optional dependencies (deephaven#560)
77b5853
@bmingles bmingles mentioned this issue Jun 17, 2024
Merged
bmingles added a commit to bmingles/deephaven-plugins that referenced this issue Jun 17, 2024
@bmingles
Hoisting swc optional dependencies (deephaven#560)
de4b3b8
bmingles added a commit that referenced this issue Jun 17, 2024
@bmingles
build: Hoisting rollup and esbuild optional dependencies (#562)
4d005d2 
Rollup and esbuild optional dependencies were removed from the
package-lock by this PR:
#502

Adding them back to top level `optionalDependencies` to ensure they are
available

For reference to get this list I:
1. used `vscode` file history to find the package-lock.json diff that
removed the dependencies.
2. copy / pasted the removed blocks of `esbuild` and `rollup`
dependencies into a new `.json` file
3. Used `jq` to flatten the names + versions `cat myfile.json | jq
'with_entries(.value = .value.version)'`
4. Tweak / copy / pasted into optionalDependencies

resolves #560
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bmingles bmingles

Labels
enhancement New feature or request triage
Projects
None yet
Milestone
No milestone
2 participants
@bmingles @mattrunyon