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

Change runtime to bun #405

Closed
wants to merge 3 commits into from
Closed

Change runtime to bun #405

wants to merge 3 commits into from

Conversation

KaiVolland
Copy link
Contributor

@KaiVolland KaiVolland commented Sep 12, 2024

This changes the runtime and creation of the binaries from node to bun.

It also adds options to read ArcGIS styles via geostyler-lyrx-parser.

⚠️ As this changes the runtime of this project this will result in a new major version.

@marcjansen
Copy link
Contributor

Are the changes to the installation guide (or the part usage without installation) in the README needed? It will still be possibly to run this CLI without bun, or not?

package.json Outdated Show resolved Hide resolved
@KaiVolland
Copy link
Contributor Author

Are the changes to the installation guide (or the part usage without installation) in the README needed? It will still be possibly to run this CLI without bun, or not?

I was not able to get the module resolution working with node and the current state of the geostyler packages. For some reason node has issues resolving relative pathes in the node_modules. bun seems to be smarter here.

I also struggle validating npx and bunx without publishing the package. 🤔

Copy link
Collaborator

@geographika geographika left a comment

Choose a reason for hiding this comment

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

I only learnt of bun at the codesprint, so I've no useful opinion on switching from one to the other. However, if it addresses #393 then I am all for it!

.github/workflows/on-push-main.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
src/logHelper.ts Show resolved Hide resolved
@jansule
Copy link
Contributor

jansule commented Sep 16, 2024

You also might want to rephrase your commit messages so that the support for openlayers does not appear in the changelog

BREAKING CHANGE: changes runtime from node to bun
lyrx can only be written
@marcjansen
Copy link
Contributor

marcjansen commented Sep 16, 2024

Are the changes to the installation guide (or the part usage without installation) in the README needed? It will still be possibly to run this CLI without bun, or not?

I was not able to get the module resolution working with node and the current state of the geostyler packages. For some reason node has issues resolving relative pathes in the node_modules. bun seems to be smarter here.

I also struggle validating npx and bunx without publishing the package. 🤔

I am +0 on the change of the runtime, so not blocking.

I really would like to stress that installation via node / npx should still work even after we switch to no longer using or endorsing them. Maybe you can create a prerelease and test this?

@KaiVolland
Copy link
Contributor Author

Are the changes to the installation guide (or the part usage without installation) in the README needed? It will still be possibly to run this CLI without bun, or not?

I was not able to get the module resolution working with node and the current state of the geostyler packages. For some reason node has issues resolving relative pathes in the node_modules. bun seems to be smarter here.
I also struggle validating npx and bunx without publishing the package. 🤔

I am +0 on the change of the runtime, so not blocking.

I really would like to stress that installation via node / npx should still work even after we switch to no longer using or endorsing them. Maybe you can create a prerelease and test this?

After this change, node' and npx' will no longer work.

@ahocevar
Copy link
Contributor

Isn't this a change more radical than it needs to be? Using bun to create the standalone binaries is fine. But what's the benefit of getting rid of a build that works with node? I just created #407, by creating a single javascript file that runs fine everywhere, using rollup. There's no need to get rid of node support, while still being able to use bun.

@KaiVolland
Copy link
Contributor Author

This is superseeded by #407. I'll create a new PR with the changes that are still useful later.

@KaiVolland KaiVolland closed this Sep 18, 2024
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.

6 participants