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

Add NPM package-lock.json #192

Merged
merged 4 commits into from
May 24, 2024
Merged

Add NPM package-lock.json #192

merged 4 commits into from
May 24, 2024

Conversation

aSemy
Copy link
Collaborator

@aSemy aSemy commented May 24, 2024

The Yarn package lock file was removed in #187, but I didn't add the new NPM package-lock.json, but it is required.

  • Re-add the 'relocate lockfile dir' logic.
  • Enable reportNewPackageLock so KGP creates an error if the lockfile is missing (otherwise it generates a new one and doesn't complain, which obscures the fact that one is missing!)
  • Add workaround for https://youtrack.jetbrains.com/issue/KT-67442

The Yarn package lock file was removed in #187,
but I didn't add the new NPM `package-lock.json`, but it is required.

Re-add the 'relocate lockfile dir' logic.

Enable reportNewPackageLock so KGP creates an error if the lockfile is missing (otherwise it generates a new one and doesn't complain, which obscures the fact that one is missing!)
@aSemy aSemy force-pushed the fix/kotlin-js-store branch from 2e834ec to 0969b8f Compare May 24, 2024 14:04
@aSemy aSemy marked this pull request as draft May 24, 2024 14:23
build.gradle.kts Outdated
@@ -89,3 +93,41 @@ dokkatoo {
}
}
}

tasks.withType<LockCopyTask>().configureEach {
Copy link
Owner

Choose a reason for hiding this comment

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

Wondering if we haven't reach the tipping point where we just pragmatically use the default yarn.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, using Yarn is an option. I'd like to workaround it though. It's a pretty simple find-replace fix, and it's nice not to need an additional JS tool.

I think this might be a NPM problem npm/cli#6379 so it might be a while before KGP can fix it.

Copy link
Owner

Choose a reason for hiding this comment

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

Isn't npm/yarn something internal here, a transitive dependency of Kotlin, that manifests itself only in the lockfiles? Do we need to actually maintain the tools? I'm fine with npm just to get us unblocked, but why did the Kotlin team decide to use yarn as the default?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question, I don't know why Kotlin/JS uses Yarn as a default. The K2 release notes imply that it will be removed as the default though.

I would speculate that Yarn was selected because it was better than NPM, but that was 7+ years ago, and NPM has since caught up.

I also think that the root cause of the problem is Gradle. It's supposed to be a language-agnostic build tool, so why can't Gradle download and verify NPM dependencies? Why do we need a lockfile for JS dependencies when Gradle already has its own lockfiles? It's because Gradle only supports Maven repos, provides no options for anyone to implement other package sources. If it did, then the experience would be much better.

I've had an idea rattling around about creating a Gradle plugin that would spawn a background daemon running both a NPM proxy server and a localhost webserver that would present the NPM dependencies as Maven Coordinates. The Gradle TypeScript plugin does something like this.

@aSemy aSemy force-pushed the fix/kotlin-js-store branch from 2462f99 to ef581b1 Compare May 24, 2024 17:31
@aSemy aSemy marked this pull request as ready for review May 24, 2024 17:51
@krzema12 krzema12 merged commit c3c5034 into main May 24, 2024
4 checks passed
@krzema12 krzema12 deleted the fix/kotlin-js-store branch May 24, 2024 18:09
@aSemy
Copy link
Collaborator Author

aSemy commented May 25, 2024

Well... it looks like this workaround didn't work. There's a failure in #191

https://github.com/krzema12/snakeyaml-engine-kmp/actions/runs/9228285746/job/25392037275?pr=191#step:5:489

The diff view is very useful though!

> Task :kotlinStorePackageLock FAILED
--- D:/a/snakeyaml-engine-kmp/snakeyaml-engine-kmp/build/js/package-lock.json
+++ D:/a/snakeyaml-engine-kmp/snakeyaml-engine-kmp/gradle/kotlin-js-store/package-lock.json
@@ -10,8 +10,8 @@
       "workspaces": [
         "packages/snakeyaml-engine-kmp",
         "packages/snakeyaml-engine-kmp-test",
-        "packages\\snakeyaml-engine-kmp-wasm-js",
-        "packages\\snakeyaml-engine-kmp-wasm-js-test",
+        "packages/snakeyaml-engine-kmp-wasm-js",
+        "packages/snakeyaml-engine-kmp-wasm-js-test",
         "packages_imported/kotlin-test-js-runner/0.0.1"
       ],
       "devDependencies": {}

@krzema12
Copy link
Owner

@aSemy let's try with yarn then?

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.

2 participants