-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
build: add deploy preview of the Vega editor #3925
Conversation
e4e9ddb
to
cfb8f33
Compare
cfb8f33
to
8aeb370
Compare
Deploying vega with Cloudflare Pages
|
5429e38
to
4858d26
Compare
4858d26
to
2ea19fd
Compare
"workspaces": [ | ||
"packages/*" | ||
], | ||
"engines": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"engines": { | ||
"node": ">=18.18.0" | ||
}, | ||
"packageManager": "yarn@1.22.19" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this, we get an auto-upgrade to yarn 3.
16:26:44.651 | Detected the following tools from environment: yarn@3.6.3, nodejs@18.17.1
-- | --
16:26:44.651 | Installing project dependencies: yarn
16:26:45.212 | ➤ YN0070: Migrating from Yarn 1; automatically enabling the compatibility node-modules linker 👍
16:26:45.212 |
16:26:45.342 | ➤ YN0000: ┌ Resolution step
16:26:46.174 | ➤ YN0032: │ canvas@npm:2.11.2: Implicit dependencies on node-gyp are discouraged
16:26:46.421 | ➤ YN0032: │ nan@npm:2.17.0: Implicit dependencies on node-gyp are discouraged
16:26:47.253 | ➤ YN0032: │ fsevents@npm:2.3.2: Implicit dependencies on node-gyp are discouraged
16:26:49.121 | ➤ YN0061: │ @npmcli/move-file@npm:2.0.1 is deprecated: This functionality has been moved to @npmcli/fs
16:26:49.642 | ➤ YN0061: │ request@npm:2.88.2 is deprecated: request has been deprecated, see https://github.com/request/request/issues/3142
16:26:49.660 | ➤ YN0061: │ uuid@npm:3.4.0 is deprecated: Please upgrade to version 7 or higher. Older versions may use Math.random() in certain circumstances, which is known to be problematic. See https://v8.dev/blog/math-random for details.
16:26:49.815 | ➤ YN0061: │ har-validator@npm:5.1.5 is deprecated: this library is no longer supported
16:26:50.972 | ➤ YN0032: │ @parcel/watcher@npm:2.0.4: Implicit dependencies on node-gyp are discouraged
16:26:51.046 | ➤ YN0032: │ node-addon-api@npm:3.2.1: Implicit dependencies on node-gyp are discouraged
16:27:01.937 | ➤ YN0002: │ @nrwl/devkit@npm:16.10.0 doesn't provide nx (p048f2), requested by @nx/devkit
16:27:01.937 | ➤ YN0060: │ root-workspace-0b6124@workspace:. provides eslint (p1305c) with version 8.53.0, which doesn't satisfy what typescript-eslint and some of its descendants request
16:27:01.938 | ➤ YN0000: │ Some peer dependencies are incorrectly met; run yarn explain peer-requirements <hash> for details, where <hash> is the six-letter p-prefixed code
16:27:01.942 | ➤ YN0000: └ Completed in 16s 599ms
16:27:02.015 | ➤ YN0000: ┌ Post-resolution validation
16:27:02.016 | ➤ YN0028: │ The lockfile would have been modified by this install, which is explicitly forbidden.
99f2f6f
to
550a7ee
Compare
550a7ee
to
5c08192
Compare
86866e9
to
7a8c8f0
Compare
0d17c99
to
ae5c73c
Compare
"license": "lerna exec -- cp ../../LICENSE .", | ||
"release": "yarn license && lerna publish from-package", | ||
"serve": "serve packages/vega/ -l 8080", | ||
"test": "yarn test:no-lint && yarn lint", | ||
"test:no-lint": "lerna run test --ignore vega-typings && lerna run test --scope vega-typings", | ||
"lint": "ESLINT_USE_FLAT_CONFIG=true yarn eslint -c eslint.config.mjs .", | ||
"format": "yarn lint --fix", | ||
"postinstall": "yarn data" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we must keep this as a postinstall step and can't move this to docsbuild, an alternate approach would be to replace this with a guard so that I can disable the postinstall step with an env variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convinced myself it would be less of a change for existing devs to take this route.
package.json
Outdated
"node": ">=18.18.0" | ||
}, | ||
"packageManager": "yarn@1.22.19", | ||
"volta": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommended node version manager - can drop this if it's distracting / people prefer to bring their own node version management solution
As it stands, you won't be able to build the project if you have an older version (like node 18.15) running locally.
f2886e1
to
d4bacf0
Compare
d4bacf0
to
9c7fb3f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove logs of course.
package.json
Outdated
"node": ">=18.18.0" | ||
}, | ||
"packageManager": "yarn@1.22.19", | ||
"volta": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove this. I use Volta but not everyone does and I like to use node 20+ since 18 is pretty old now.
echo "Installing Vega" | ||
|
||
# Link every package to make sure the editor uses the local version | ||
for package in packages/*; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't lerna do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lerna can handle running a command in every package, but for linking it delegates to yarn / npm https://lerna.js.org/docs/legacy-package-management#migrating-from-lerna-bootstrap-lerna-add-and-lerna-link-in-lerna-v7-and-later
It's supposed to prefer the local version if that packages the package version but the bug we're currently facing in vega is that multiple versions of the subpackages are active at once
The yarn monorepo solution for this is the workspaces:*
protocol so we can make the package prefer the version in this monorepo, the specific version number would be replaced on the publish step. To use it we would have to upgrade to yarn v2 and up.
I think that move is worth it, but didn't want to get that tied up in the scope of this PR.
scripts/build-editor-preview.sh
Outdated
cd editor | ||
yarn --frozen-lockfile --ignore-scripts | ||
|
||
# Make sure we prefer the local version to the one from npm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a hack. What goes wrong if we didn't have this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd agree this is a hack, ideally we shouldn't need to install these from NPM
Ideally we can remove this after we run through and make sure there's only 1 version of each package ( @lsh mentioned he might get to look into it in the vega slack , I didn't open a new issue for it )
set -eo pipefail | ||
|
||
# Run this script as long as the env variable | ||
# DISABLE_POSTINSTALL_SCRIPT is not set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the CI dashboard config: https://dash.cloudflare.com//pages/view/vega/settings/environment-variables
Motivation
Changes
Testing
vega
and a subpackage likevega-view
are both visible.