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

Free the npm package from third party dependencies #51

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

lydell
Copy link

@lydell lydell commented Jan 6, 2023

Status

I have contacted @zwilias on Slack, but haven’t got any response.

In the mean time, I’ve published this as @lydell/elm-json on npm.

PR

This is:

Read the PR description as well as the changes in PUBLISHING.md in elm/compiler#2287 for full details on how it works, the pros, the (minor) cons, and the full compatibility story.

Fixes #49.

Here’s how to publish:

Repeat this for all the binary packages in packages/. This uses packages/elm-json-darwin-x64 as an example.

  1. Go to the folder: cd packages/elm-json-darwin-x64

  2. Copy the appropriate binary to ./elm-json. For Windows: ./elm-json.exe

  3. Double-check that you put the right binary in the right package: file elm-json

  4. Double-check that the file is executable: ls -l elm-json

  5. In package.json of the binary package, bump the version for example to "0.2.14".

  6. In package.json of the main npm package, update "optionalDependencies" to point to the bumped version. For example: "@zwilias/elm-json-darwin-x64": "0.2.14".

    Note: Pin the versions of the binary packages exactly – no version ranges. This means that installing [email protected] installs the exact same bytes in two years as today.

  7. Publish the package: npm publish --access=public

    --access=public is needed because scoped packages are private by default.

Then publish the main npm package like you did before.

This is good to think about the first time: https://github.com/avh4/elm-format/pull/781/files#r1089739625

@@ -5,22 +5,23 @@
"author": "Ilias Van Peer",
"license": "MIT",
"repository": "zwilias/elm-json",
"preferGlobal": true,
Copy link
Author

Choose a reason for hiding this comment

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

preferGlobal has been deprecated for a long time, and is even removed in newer npm versions.

Comment on lines -19 to -22
"linux-arm": root + "-armv7-unknown-linux-musleabihf.tar.gz",
"linux-arm64": root + "-armv7-unknown-linux-musleabihf.tar.gz",
"win32-x64": root + "-x86_64-pc-windows-msvc.tar.gz",
"win32-ia32": root + "-x86_64-pc-windows-msvc.tar.gz"
Copy link
Author

Choose a reason for hiding this comment

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

Just noticed that the old version supports linux-arm (32-bit) and win32-ia32 as well, and that both the 32-bit and 64-bit platforms use the same binaries?

I can easily add support for those, but just wanted to check something with you first. I don’t know much about 32-bit and 64-bit stuff. I thought that you needed different binaries for them? Is it a mistake in this code, or does it actually work?

Copy link
Author

Choose a reason for hiding this comment

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

I asked on Discord, and the linux ARM binary is 32-bit but also works on 64-bit ARM, so I added support for that.

It’s still unclear about Windows, though.

Copy link
Author

Choose a reason for hiding this comment

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

Update regarding Windows:

Mario Rogic
I was thinking about this combo, which I have no idea how that works:
"win32-ia32": root + "-x86_64-pc-windows-msvc.tar.gz"

BendingBender
no this one doesn't make any sense for me

BendingBender
At least what I saw in the build scripts in elm-json it looks like it's really the 64-bit x86 version that's being served to 32-bit systems. As you said, it's quite likely that it won't work but if no one ever complains, then I suppose no one actually uses it.
The good thing using rust in case of elm-json is that one doesn't need a windows system to cross-compile for windows, all the builds for all the OS' are done on a linux system and there would actually be no problem to build a 32-bit x86 windows binary. Which is completely pointless if Elm itself isn't built for this architecture.


Current conclusion: The PR is good as it is, no need to add code to “support” 32-bit Windows. Both the old and new package fail in different ways on 32-bit Windows.

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.

Critical severity vulnerability - json-schema (via binwrap)
1 participant