-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
framework api: init / work in progress #13215
Conversation
❌ @paperdave, your commit fa7abdd has 10 failures in test/js/web/streams/streams.test.js - 1 failing on 🍎 13 x64test/js/node/test/parallel/fs-watch-recursive-add-file.test.js - 1 failing on 🍎 14 x64test/js/bun/shell/leak.test.ts - 1 failing on 🪟 x64-baselinetest/js/node/fs/fs.test.ts - 1 failing on 🪟 x64-baselinetest/js/node/fs/fs.test.ts - 1 failing on 🪟 x64test/js/bun/http/fetch-file-upload.test.ts - 1 failing on 🪟 x64-baselinetest/js/bun/http/bun-serve-static.test.ts - 4 failing on 🍎 13 aarch64test/js/web/timers/setInterval.test.js - 1 failing on 🍎 14 aarch64test/cli/hot/watch.test.ts - 1 failing on 🪟 x64-baselinetest/cli/hot/watch.test.ts - 1 failing on 🪟 x64test/cli/watch/watch.test.ts - 2 failing on 🪟 x64-baselinetest/cli/watch/watch.test.ts - 2 failing on 🪟 x64test/bundler/bun-build-api.test.ts - 1 failing on 🪟 x64-baselinetest/bundler/bun-build-api.test.ts - 1 failing on 🪟 x64 |
clang-tidy nits are fixed! Thank you. |
try pool.start( | ||
this, | ||
thread_pool, | ||
); | ||
|
||
return generator; | ||
// sanity checks for kit | ||
if (this.bundler.options.output_format == .internal_kit_dev) { |
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.
should this be wrapped in if (isDebug)
?
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.
we can, but i don't want the bundler to ever continue if these options are misconfigured. these checks are not expensive.
const import_record_index = p.addImportRecordByRange(.stmt, logger.Range.None, "react-refresh/runtime"); | ||
|
||
const Item = if (hot_module_reloading) B.Object.Property else js_ast.ClauseItem; | ||
|
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.
const num_items = 1 + @intFromBool(p.react_refresh.register_used) + @intFromBool(p.react_refresh.signature_used);
and use it for items.initCapacity
and declared_symbols.ensureTotalCapacity
?
src/js_parser.zig
Outdated
none: void, | ||
bun_dev: Expr, | ||
bun_js: void, | ||
const WrapMode = union(enum) { |
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.
should this just be an enum?
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.
yea this can now be an enum since bun_dev was deleted
}, | ||
if (have_custom_hooks) { | ||
// () => [useCustom1, useCustom2] | ||
args[3] = p.newExpr(E.Arrow{ |
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 is out of bounds if !have_force_arg
and have_custom_hooks
?
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.
not possible because have_force_arg is defined in terms of have_custom_hooks
const have_custom_hooks = ctx.user_hooks.count() > 0;
const have_force_arg = have_custom_hooks or p.react_refresh.force_reset;
// the file. That is also why tree-shaking is disabled. | ||
if (p.options.features.hot_module_reloading) { | ||
bun.assert(!p.options.tree_shaking); | ||
bun.assert(p.options.features.hot_module_reloading); |
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.
it's harmless but this assert isn't necessary
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 catch
This is initial work for the "Framework API", internally code named "Kit". This name is not final and just something to give me a fresh namespace for new code.
This configures Kit to be compiled as a part of canary to reduce bit-rot. I will periodically submit follow-up PRs expanding the feature set.
There are many changes in the JS parser and Bundler to
BundleThread
has been generalized to take a type of the completion struct (and moved tobundle_v2.BundleThread(T)
). This allows Kit to re-use a lot ofBun.build
's asynchronous and queued bundling features. The completion struct has two methods,configureBundler
andcompleteOnBundleThread
, which is all the needed abstraction to re-use that code.JSPromise
gets anunwrap
function to read a niceunion(enum)
--react-fast-refresh
. I exposed this flag so I can manually test it myself, in isolation from the hot module transforms.options.OutputFormat
in favor ofoptions.Format