-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: change access-api wrangler.toml to be no_bundle=false. use wrangler bundling #739
Changes from 21 commits
1ffb681
7cf6278
4ec1075
53ff41f
92a89d3
9570179
6927148
27eff7b
8465228
df8d799
87bd214
9cb44c3
ba143bd
f3446a0
50ded92
25aecb0
6f16f09
fb13a84
0be6257
19b7cd3
3cecdf6
7aabcc0
1d7f23e
3af1c35
d58f3b6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
.dev.vars |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
/* eslint-disable unicorn/prefer-top-level-await */ | ||
/* eslint-disable no-console */ | ||
/* eslint-disable unicorn/prefer-module */ | ||
/** | ||
* @file | ||
* use this at build time (node.js) to create files that can be | ||
* imported at runtime (maybe workerd). | ||
*/ | ||
import sade from 'sade' | ||
import * as url from 'node:url' | ||
import { getReleaseName } from '../src/utils/release.node.js' | ||
import path from 'path' | ||
import { fileURLToPath } from 'url' | ||
// @ts-ignore | ||
import git from 'git-rev-sync' | ||
|
||
function __dirname() { | ||
return path.dirname(fileURLToPath(import.meta.url)) | ||
} | ||
|
||
if (import.meta.url.startsWith('file:')) { | ||
const modulePath = url.fileURLToPath(import.meta.url) | ||
if (process.argv[1] === modulePath) { | ||
main().catch((error) => { | ||
throw error | ||
}) | ||
} | ||
} | ||
|
||
async function main(argv = process.argv) { | ||
const cli = sade('access-api-release') | ||
cli | ||
.command('name', '', { default: true }) | ||
.option('--env', 'Environment', process.env.ENV) | ||
.action((opts) => { | ||
const releaseName = getReleaseName(opts.env) | ||
console.log(releaseName) | ||
}) | ||
cli | ||
.command('esm', 'print release info as ES Module string') | ||
.option('--env', 'Environment', process.env.ENV) | ||
.action((opts) => { | ||
const lines = [ | ||
`/** @type {string|undefined} */`, | ||
`export const gitRevShort = '${git.short(__dirname())}'`, | ||
`/** @type {string|undefined} */`, | ||
`export const name = '${getReleaseName(opts.env)}'`, | ||
] | ||
console.log(lines.join('\n')) | ||
}) | ||
cli.parse(argv) | ||
} | ||
Comment on lines
+30
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is neat, but note that our other projects do this by injecting these in as variables into the bundle at build time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They do that using If there's another way that works that seems better, lmk and I can try to implement. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
/** | ||
* @file | ||
* This file MAY be rewritten during build | ||
* to make some buildtime variables available at runtime. | ||
*/ | ||
/** @type {string|undefined} */ | ||
export const gitRevShort = undefined | ||
/** @type {string|undefined} */ | ||
export const name = undefined |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
/** | ||
* @file | ||
* utils related to sentry that rely on node. | ||
* These might be used at build time, | ||
* but won't work at run-time in cloudflare workers | ||
*/ | ||
/* eslint-disable no-console */ | ||
/* eslint-disable unicorn/prefer-module */ | ||
|
||
// @ts-ignore | ||
import git from 'git-rev-sync' | ||
import path from 'path' | ||
import { fileURLToPath } from 'url' | ||
import { createRequire } from 'node:module' | ||
|
||
const packageJson = createRequire(import.meta.url)('../../package.json') | ||
const __dirname = () => path.dirname(fileURLToPath(import.meta.url)) | ||
|
||
/** | ||
* Create a string to be used for the sentry release value | ||
* | ||
* @param {string} [env] - environment name e.g. 'dev' | ||
* @param {object} pkg - package.json info | ||
* @param {string} pkg.name | ||
* @param {string} pkg.version | ||
* @param {string} gitShort - git-rev-parse short value | ||
* @returns {string} release name e.g. `@web3-storage__access-api@6.0.0-staging+92a89d3` | ||
*/ | ||
export function getReleaseName( | ||
env, | ||
pkg = packageJson, | ||
gitShort = git.short(__dirname()) | ||
) { | ||
const version = `${pkg.name}@${pkg.version}-${env}+${gitShort}`.replace( | ||
'/', | ||
'__' | ||
) | ||
return version | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,15 +1,12 @@ | ||
# Development | ||
name = "w3access-local" | ||
account_id = "fffa4b4363a7e5250af8357087263b3a" | ||
main = "./dist/worker.js" | ||
main = "./src/index.js" | ||
|
||
# Compatibility flags https://github.com/cloudflare/wrangler/pull/2009 | ||
compatibility_date = "2022-09-28" | ||
compatibility_flags = ["url_standard"] | ||
|
||
# We need to let wrangler bundle while D1 is in Beta | ||
no_bundle = false | ||
|
||
[[kv_namespaces]] | ||
binding = "SPACES" | ||
id = "e9fad7e04b254bf49206e08e50074387" | ||
|
@@ -28,43 +25,52 @@ database_id = "7c676e0c-b9e7-4711-97c8-7b1c8eb229ae" | |
[[r2_buckets]] | ||
binding = "DELEGATIONS_BUCKET" | ||
bucket_name = "w3up-delegations-dev-0" | ||
preview_bucket_name = "w3up-delegations-dev-0" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. some changes in this file seem unrelated to bundling / getting stack traces to work. not a blocker, but can you confirm why these changes are needed in this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one was suggested by |
||
|
||
[vars] | ||
ENV = "dev" | ||
DEBUG = "true" | ||
DID = "did:web:local.web3.storage" | ||
PRIVATE_KEY="MgCYWjE6vp0cn3amPan2xPO+f6EZ3I+KwuN1w2vx57vpJ9O0Bn4ci4jn8itwc121ujm7lDHkCW24LuKfZwIdmsifVysY=" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the same value from repo /.env.tpl , so it is already public |
||
UPLOAD_API_URL = "https://up.web3.storage" | ||
|
||
[build] | ||
command = "scripts/cli.js build" | ||
watch_dir = "src" | ||
|
||
[miniflare] | ||
d1_persist = ".wrangler/miniflare" | ||
|
||
rules = [ | ||
{ type = "Text", globs = ["package.json"], fallthrough = true } | ||
] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what dis? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this was required for cloudflare workers to be able to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think I need it anymore, since now release.node.js imports it in node at build time, and writes a rendered value for cloudflare to import via ESM (so it doesn't need to import the package.json to build the rendered version). I'll remove it |
||
|
||
# Dev | ||
[env.dev] | ||
name = "w3access-dev" | ||
workers_dev = true | ||
vars = { ENV = "dev", DEBUG = "false", DID = "did:web:dev.web3.storage", UPLOAD_API_URL = "https://staging.up.web3.storage" } | ||
build = { command = "scripts/cli.js build --env dev", watch_dir = "src" } | ||
kv_namespaces = [ | ||
{ binding = "SPACES", id = "5697e95e1aaa436788e6d697fd3350be" }, | ||
{ binding = "VALIDATIONS", id = "ea17f472b37a43d29c1faf7af9512e03" }, | ||
] | ||
d1_databases = [ | ||
{ binding = "__D1_BETA__", database_name = "access-dev", database_id = "4145a261-e54c-411d-a001-050fc30e4678" }, | ||
] | ||
unsafe = { bindings = [ | ||
{ type = "analytics_engine", dataset = "W3ACCESS_METRICS", name = "W3ACCESS_METRICS" }, | ||
] } | ||
|
||
[[env.dev.d1_databases]] | ||
binding = "__D1_BETA__" | ||
database_name = "access-dev" | ||
database_id = "7f5c4ec7-610b-4885-b9f7-0886ce0639f6" | ||
|
||
[[env.dev.r2_buckets]] | ||
binding = "DELEGATIONS_BUCKET" | ||
bucket_name = "w3up-delegations-dev-0" | ||
preview_bucket_name = "w3up-delegations-dev-0" | ||
|
||
# Staging | ||
[env.staging] | ||
name = "w3access-staging" | ||
workers_dev = true | ||
build = { command = "scripts/cli.js build --env staging", watch_dir = "src" } | ||
kv_namespaces = [ | ||
{ binding = "SPACES", id = "b0e5ca990dda4e3784a1741dfa28a52e" }, | ||
{ binding = "VALIDATIONS", id = "b13f07c88fe848db9ccf651a0fea3fb6" }, | ||
|
@@ -90,7 +96,6 @@ bucket_name = "w3up-delegations-staging-0" | |
[env.production] | ||
name = "w3access" | ||
routes = [{ pattern = "access.web3.storage", custom_domain = true }] | ||
build = { command = "scripts/cli.js build --env production", watch_dir = "src" } | ||
kv_namespaces = [ | ||
{ binding = "SPACES", id = "5437954e8cfd4f7d98557132b0a2e93f" }, | ||
{ binding = "VALIDATIONS", id = "fb7cf10c725f45948321e88b8cb168ad" }, | ||
|
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.
my assumption here is that the step before should already generate these as part of the wrangler action. I'd feel more comfortable here if the artefacts are build once and then sent to the places they need to go, rather than relying on the build definitely always being reproducible (it should be, but, if why tempt fate in our error reporting)
what does
SENTRY_UPLOAD
do in that previous step? is it unused, or does wrangler-action actually upload a thing to sentry (that would be surprising)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.
Nothing atm. (because now
wrangler.toml
doesn't define a custom build command that might read it).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.
Good suggestion. I had thought so too, and tried that, but iirc it didn't write the files.
Let me see if it's just because I need to pass
--outdir=dist
on the previous non-dry-run publish.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.
Added a commit to only invoke
wrangler publish
once (viawrangler-action
with custom command to pass--outdir dist
) 3af1c35