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

Better usage instructions and less build requirements (rm+cp instead of rsync) #1444

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

schaetzc
Copy link
Contributor

@schaetzc schaetzc commented Sep 15, 2024

This pull request improves documentation and build scripts for users who just want to run vega-editor locally.
See #1305 for more information.

rsync is no longer needed to run install/build/run vega-editor. Using standard rm and cp commands instead of rsync is not only more portable, but was also faster on my system. I ran multiple repeated tests ...

  • on an SSD and on a 10 year old HDD,
  • with and without dropping caches (sudo sh -c 'sync && echo 3 > /proc/sys/vm/drop_caches') in between repeats,
  • for situations where the target directories were only updated, and situations where they were initialized for the first time.

Sometimes rsync took 10 times as long. But it does not really matter. Copying always took < 0.5 s while the downloads from the internet were the main slowdown for me.

This pull request also fixes some bugs in scripts/vendor.sh, e.g.

  • Spaces and special symbols * ? [] $ ` are now supported in the (absolute) path of all files involved.
  • When an example is removed from the source dirs, it is now removed in the (existing) target dirs too (basically rsync's --delete option, which wasn't used so far)

Documentation was improved, so that other requirements (which are not changed in this pull request) are explicitly mentioned.

The oddity that the working copy cannot be directly below the root directory comes from below line in package.json ...

"scripts": {
  "start": "PARCEL_BUILD_COMMIT_HASH=$(git rev-parse HEAD) parcel --watch-dir .. index.html",

The parent directory .. could be / which parcel does not like. In such a situation it prints [Error: Invalid argument] and terminates with exit code 1.
This could be fixed by changing parcel --watch-dir .. to parcel --watch-dir . or leaving out the option completely. However, I am not sure if that would have negative effects on the developers. I assume they have multiple vega subprojects checked out in one shared top level directory vega/ (e.g. vega/editor, vega/lite, ...) and actually want the .. . Therefore I only commented on it.

Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Thank you for also fixing other things. Just two comments and otherwise good to go.

README.md Outdated
@@ -10,21 +10,29 @@ You can reset the Vega Editor by going to https://vega.github.io/editor/#/reset

## Usage Instructions

To run the editor locally, you must first install the dependencies and then launch a local web server. We assume you have [yarn](https://yarnpkg.com/) installed.
### In Short
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename this to "running with docker"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to "Run Vega-Editor With Docker"

README.md Outdated

1. Install the dependencies:
### In Detail
Copy link
Member

Choose a reason for hiding this comment

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

And this to "development setup"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to "Development Setup"

README.md Outdated
### In Detail
We assume you have the following tools installed:
- [yarn](https://yarnpkg.com/)
- The older `node` version 21, because of an incompatibility in one of our dependencies, see [node-canvas issue](https://github.com/Automattic/node-canvas/issues/2377)
Copy link
Member

Choose a reason for hiding this comment

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

This feels out of place in the readme since we can hopefully fix this going forward and will forget to update it here. What if we set the node version in the package.json instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I thought the same too, but wasn't sure where to put it instead.

What if we set the node version in the package.json instead?

If that is possible and an acceptable practice, that would be great. I have no experience with tools like node and yarn, and cannot judge that.
I assumed there was a reason node wasn't already managed through package.json or yarn.lock, e.g. that the tools installing/updating the packages cannot modify the node installation while they run inside it.

I removed the hint without changing anything else. I'd be happy if you could handle that. But I'm also ok with merging as is. By reading the docker command, the information which node version to use is still there.

hopefully fix this going forward

I was hoping so too. It seems there will be no release for node-canvas 2.x anymore; they already have a release preview for 3.0. So to make use of the upstream fix, vega-editor probably has to upgrade to node-canvas 3.x once its officially released. I have no idea, if there are any breaking changes that could affect vega-editor.

@schaetzc
Copy link
Contributor Author

Thank you for the review.

I --amended my last git commit, to adapt the subsection titles as suggested, and remove the overly specific detail regarding the node version.
I assume pressing the "resolve conversation" buttons in this pull request is up to you, right?

@domoritz
Copy link
Member

Thank you

@domoritz domoritz merged commit 1484a09 into vega:master Sep 19, 2024
1 check passed
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