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

Bun support and linting configuration refactor #1225

Merged
merged 24 commits into from
Sep 14, 2024
Merged

Conversation

DarkGL
Copy link
Collaborator

@DarkGL DarkGL commented May 11, 2024

This is work in progress.

I'm still having some problems running bun, and I'm looking for help to fix it.

https://github.com/DarkGL/typescript-runtime-type-benchmarks/actions/runs/9016482516/job/24842376959

DarkGL added 11 commits April 26, 2024 13:48
A new GitHub Actions workflow job for building with Bun has been added. This includes setup, installation, linting, and testing steps. A lockfile for Bun was also added to the ignore list.
Changed the variable name for setting up Bun version to match the correct matrix value.
Added a dependency for the 'build-bun' job to run after the 'build' job in the workflow.
@moltar
Copy link
Owner

moltar commented May 11, 2024

@nin-jin Can you help @DarkGL with this?

screenshot-20240511T151635-F0UVb8A0@2x

@DarkGL
Copy link
Collaborator Author

DarkGL commented Jun 1, 2024

Tried with 1.1.10 but still the same

@kravetsone
Copy link

@nin-jin Can you help @DarkGL with this?

screenshot-20240511T151635-F0UVb8A0@2x

Not Bun-only issue

I create file index.mjs

import {
	$mol_data_email,
	$mol_data_integer,
	$mol_data_optional,
	$mol_data_pipe,
	$mol_data_record,
	$mol_data_string,
	$mol_data_variant,
} from "mol_data_all";

const PersonDTO = $mol_data_record({
	name: $mol_data_string,
	age: $mol_data_optional($mol_data_integer),
	birthday: $mol_data_pipe($mol_data_string),
});

// Derived Type
const UserDTO = $mol_data_record({
	...PersonDTO.config,
	phone: $mol_data_variant($mol_data_string, $mol_data_integer),
	mail: $mol_data_email,
});

// Ensure this is a User
const jin = UserDTO({
	name: "Jin",
	age: 33,
	birthday: "1984-08-04T12:00:00Z",
	phone: 791234567890,
	mail: "[email protected]",
});

console.log(jin);

And run it with node index.mjs

PS C:\Users\krave\Desktop\test\tests\mol-bun> node .\index.mjs
file:///C:/Users/krave/Desktop/test/tests/mol-bun/index.mjs:2
        $mol_data_email,
        ^^^^^^^^^^^^^^^
SyntaxError: The requested module 'mol_data_all' does not provide an export named '$mol_data_email'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:131:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:213:5)
    at async ModuleLoader.import (node:internal/modules/esm/loader:316:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:66:12)

Node.js v20.9.0

@kravetsone
Copy link

@nin-jin карловский почини пж ESM 🙏🙏

@kravetsone
Copy link

Yay! I've run all the benchmarks on the Bun

@kravetsone
Copy link

Yay! I've run all the benchmarks on the Bun

i use

const {
  $mol_data_tagged: Tagged,
  $mol_data_integer: Integer,
} = require("mol_data_all"); 

In bun you can mix require+import
but not in node...

@DarkGL DarkGL force-pushed the bun-support branch 2 times, most recently from 1835ee5 to eff9a37 Compare July 24, 2024 16:55
@DarkGL DarkGL closed this Jul 26, 2024
@kravetsone
Copy link

Why it closed?

@DarkGL
Copy link
Collaborator Author

DarkGL commented Jul 27, 2024

I currently don't have time to work on this, if you are interested you can pick it up :)

@nin-jin
Copy link
Contributor

nin-jin commented Sep 7, 2024

Ух ты, сколько тут страданий. Я только сейчас узнал про эту проблему. У нас экспортируются не сами сущности, а дефолтный контекст, их содержащий. Там описано, что с этим делать.

@nin-jin
Copy link
Contributor

nin-jin commented Sep 7, 2024

Где врут?

@DarkGL DarkGL reopened this Sep 7, 2024
@DarkGL
Copy link
Collaborator Author

DarkGL commented Sep 7, 2024

Great to hear that you are aware of the problem, reopened, and hopefully we can get it landed

@kravetsone
Copy link

Great to hear that you are aware of the problem, reopened, and hopefully we can get it landed

this example works fine (based on hyoo-ru/mam_mol#692 (comment))

import $ from "mol_data_all";
const {
	$mol_data_email,
	$mol_data_integer,
	$mol_data_optional,
	$mol_data_pipe,
	$mol_data_record,
	$mol_data_string,
	$mol_data_variant,
} = $;

console.log($);

const PersonDTO = $mol_data_record({
	name: $mol_data_string,
	age: $mol_data_optional($mol_data_integer),
	birthday: $mol_data_pipe($mol_data_string),
});

// Derived Type
const UserDTO = $mol_data_record({
	...PersonDTO.config,
	phone: $mol_data_variant($mol_data_string, $mol_data_integer),
	mail: $mol_data_email,
});

// Ensure this is a User
const jin = UserDTO({
	name: "Jin",
	age: 33,
	birthday: "1984-08-04T12:00:00Z",
	phone: 791234567890,
	mail: "[email protected]",
});

console.log(jin);

@DarkGL DarkGL marked this pull request as draft September 7, 2024 18:39
@DarkGL DarkGL marked this pull request as ready for review September 10, 2024 19:40
@DarkGL
Copy link
Collaborator Author

DarkGL commented Sep 10, 2024

I think it's ready for review :)

@moltar

@moltar moltar requested a review from hoeck September 10, 2024 20:25
@moltar
Copy link
Owner

moltar commented Sep 10, 2024

@DarkGL Massive work! ❤️

@hoeck lots of graphing changes on this one; please see if you have any opinions

@DarkGL DarkGL changed the title WIP: Bun support Bun support and linting configuration refactor Sep 10, 2024
@DarkGL
Copy link
Collaborator Author

DarkGL commented Sep 13, 2024

@hoeck just a friendly reminder

I want to start working on integrating deno after merging this :)

@kravetsone
Copy link

kravetsone commented Sep 13, 2024

@hoeck just a friendly reminder

I want to start working on integrating deno after merging this :)

I guess it will be harder...
But why not

@hoeck
Copy link
Collaborator

hoeck commented Sep 14, 2024

@hoeck just a friendly reminder

I want to start working on integrating deno after merging this :)

Sry for the delay, looking at it now ...

Copy link
Collaborator

@hoeck hoeck left a comment

Choose a reason for hiding this comment

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

Great work! Haven't tested it on my machine but it looks really good so far.

If you agree with some of my comments you can change the code at your will. But you don't have to.

@@ -95,7 +126,29 @@ public/
# DynamoDB Local files
.dynamodb/

# End of https://www.gitignore.io/api/node
# TernJS port file
.tern-port
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a friend of these massive templated .gitignores. Makes it hard to read for me and has the chance to bury stuff unwanted. Also never gonna use all these frameworks here.

But thats just my personal preference.

b.benchmark,
].join('-'),
}));

const nodeJsVersionCount = new Set(values.map(v => v.nodeVersion)).size;
const valuesBun = benchmarkResultsBun
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see what you did here 😁 - you basically copied all the fetching, data-preparation and filter stuff so that there are lists of node and bun benchmarks.

Maybe using a single-array with Bun | Nodejs results would result in less code overall.

But your approach works too ofc. Hats off to you for figuring out how this code works and extending it!

...b,
opsLabel: b.ops.toLocaleString('en-US'),
// artificical benchmark name to make sure its always sorted by
// benchmark and node-version
Copy link
Collaborator

Choose a reason for hiding this comment

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

node-version -> bun-version

These kinds of little mistakes happen to me too when copying code.

@hoeck hoeck merged commit 6826b79 into moltar:master Sep 14, 2024
7 checks passed
@DarkGL
Copy link
Collaborator Author

DarkGL commented Sep 14, 2024

Thanks ! I will fix those typos in next pr, and maybe figure out more general way rather than just copying 😅

@DarkGL
Copy link
Collaborator Author

DarkGL commented Sep 14, 2024

It looks like I have broken selecting default version for Node @hoeck @moltar

@kravetsone
Copy link

@hoeck just a friendly reminder

I want to start working on integrating deno after merging this :)

Great work!

@hoeck
Copy link
Collaborator

hoeck commented Sep 14, 2024

It looks like I have broken selecting default version for Node @hoeck @moltar

You're right, I've created an issue for that: #3111 (and thanks for testing your changes 😁)

The init code is using nodeVersion as a key to set the initial selected one. In the new file format this is now runtime and
runtimeVersion: https://github.com/moltar/typescript-runtime-type-benchmarks/blob/master/docs/app.tsx#L480

That wasn't caught by Typescript as the fetch response is not properly typed and/or runtime type checked (all while this repo is about runtype type checking 😆 )

Can you fix it? If you don't have the time or need help, please let me know.

@hoeck
Copy link
Collaborator

hoeck commented Sep 14, 2024

Thanks ! I will fix those typos in next pr, and maybe figure out more general way rather than just copying 😅

nah that sort/order/groupby code was not good to begin with so copying is totally fine 😄 .

I've played around with a client-side PostgresSQL database (electric-sql/pglite) lately. Maybe (just maybe) we could load all results into an sqlite db and then use that for querying / grouping / sorting instead of hand-rolling everything in JS.

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.

5 participants