-
-
Notifications
You must be signed in to change notification settings - Fork 669
General cleanup #2757
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
General cleanup #2757
Conversation
42d50cf
to
9fd90aa
Compare
785763f
to
f1fe26e
Compare
I gave up on trying to make the DTS generator faster. :( |
3b945ad
to
5fc8a8f
Compare
14887c9
to
02eb21c
Compare
Can this PR be merged now, or you still want to update it? @CountBleck |
I'm not entirely sure. |
@dcodeIO do you have any interest in using Codecov? (I've noticed it being used in Binaryen.) |
If the test is supposed to compile, but compilation fails instead, there's no point in diffing fixtures (unless you like a wall of red in your terminal). It also turns out that execution never reaches the diffing section, since writing the non-existent debug Wasm binary fails.
I don't know, this is probably a good idea.
These changes aim to make the code somewhat simpler, although it seems that the DTS generator will forever remain a mess of some sort. This commit has no effect on the contents of assemblyscript.generated.d.ts and asc.generated.d.ts. Moreover, "wrapper" files generated by the DTS generator (asc.d.ts, assemblyscript.d.ts, transform.d.ts, transform.cjs, transform.js) are now included in the tree instead of being generated, since they will never change. The transform.{c,}js files also use ES6 classes instead of functions (why not?). Also, the build.js script now imports build-dts.js instead of spawning it as a separate process. The following changes to the generator have been made: * `options.stdout` is now just a plain array * `options.main` is assumed nonexistent * `options.verbose` is assumed true * `options.sendMessage` is assumed `util.debuglog("dts")` * use `NODE_DEBUG=dts` to view the logs * `options.types` is assumed nonexistent * `options.out` is assumed nonexistent * `options.resolveModuleId` is assumed nonexistent * `options.prefix` is assumed to exist * the use of `new Promise(...)` is now removed * `import ... = require(...)` statements are assumed to never exist * double quotes are now used instead of single quotes * `else` is now on the same line as the ending brace of `if` * other style changes have been made while I'm at it * `DTS` is a constant just like `DTSLEN` * `excludesMap` is now a set called `exclusions` * `transformTypes` uses `context.factory` to create identifiers * a duplicate `currentModuleId` variable is now removed * unneeded merging of `compilerOptions` and `options` is now removed * `pathUtil.relative()` is used instead of slicing * arrow functions are used more often * (and some other things I likely forgot about)
Keeping (dev) dependencies up to date is likely a good idea, especially if later versions are (ever so slightly) faster. glob v10 removed its default export, so the `globSync` named export is now used in scripts. Since the platform path separator is now used starting from glob v9, the `posix` option is enabled to fix the resulting breakage. glob v9 also removed the sorting of results, which caused one test to fail, as the bundled standard library object had differently ordered fields, which in turn affected the order in `Program#elementsByName`. `.sort()` is used on the standard library glob in build.js to alleviate this issue (I am unsure if glob returns deterministic results).
This was my fault, and I only noticed it when cleaning up the web build, since the web build silently failed before my changes to it.
The previous commit explains why I missed this bug.
Just like the DTS build, the web build now exports functions instead of being spawned as a child process. Also, the web build no longer fails silently upon error.
This new job is very barebones at the moment. It only outputs a summary of the coverage in the terminal, since the entire coverage is a bit long and unwieldy. Perhaps Codecov or similar can be used in the future.
This was an error on my (@CountBleck) part. Co-authored-by: Congcong Cai <[email protected]>
50a15c5
to
16e4dc5
Compare
The latest packages that were added to the lockfile when resolving a rebase conflict might not have survived the rest of the rebase, so another bump is provided here instead. This bump also caused TypeScript to emit an error related to an undici-types package due to this bump, and that is also fixed here.
16e4dc5
to
dd8fd9d
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.
LGTM
I'm not sure why (not familiar with assemblyscript) but I suspect this PR broke my workflow:
Notice how it's complaining about You can reproduce this with the following steps: npm install --no-save assemblyscript
./node_modules/.bin/asc foo.ts |
This allows AssemblyScript#2786 (which fixes the regression from AssemblyScript#2757) to be published as soon as possible. Note that the main branch is the only branch that can be published, since `ref: main` is passed to actions/checkout.
Changes proposed in this pull request:
⯈ Skip fixture comparisons in tests if compiling them failed
⯈ Update esbuild
⯈ Clean up the DTS generator
⯈ Update glob
⯈ Marginally clean up the web build thingy
⯈ Add a coverage job to CI; maybe Codecov can be used on it in the future?