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

Implement tests with uvu/swc #104

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Implement tests with uvu/swc #104

wants to merge 12 commits into from

Conversation

natemoo-re
Copy link
Owner

@natemoo-re natemoo-re commented Jan 23, 2021

Alternative to #100, using uvu -r @swc-node/register to compile .ts tests on the fly without writing them to disk.

@eyelidlessness did a bunch of this legwork, so this wouldn't have happened without them!

  • Waiting on @swc-node/register@1.0.4 which includes a patch to respect jsxFactory and jsxFragmentFactory from tsconfig.
  • Fix microsite's compilation to output .js file extensions because @swc-node/register doesn't (and probably shouldn't) allow that like TypeScript does

@changeset-bot
Copy link

changeset-bot bot commented Jan 23, 2021

⚠️ No Changeset found

Latest commit: 1853843

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

💥 An error occurred when fetching the changed packages and changesets in this PR
Some errors occurred when validating the changesets config:
The package or glob expression "@example/*" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch.

@vercel
Copy link

vercel bot commented Jan 23, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

microsite – ./site

🔍 Inspect: https://vercel.com/nmoo/microsite/qremvjqc8
✅ Preview: Canceled

[Deployment for 1853843 canceled]

microsite-examples – ./

🔍 Inspect: https://vercel.com/nmoo/microsite-examples/huy9k67do
✅ Preview: Failed

[Deployment for 1853843 failed]

@natemoo-re natemoo-re mentioned this pull request Jan 23, 2021
@eyelidlessness
Copy link
Collaborator

I’m stoked to see this working! I will say I found Swc docs somewhat underwhelming, so I took another stab at an esbuild-based solution and once again sunk a bunch of time. I’m gonna let this white whale go for now and focus on getting something real done.

But I will mention I discovered a few issues along the way with how some dependencies and local modules are imported.

  • Essentially, I’m not entirely sure it’s a good idea to allow synthetic default imports when targeting esm. But if all the tooling is playing nice, I think we might be able to just live with that.

  • It looks like you’ve noticed this too, but imports with a .js suffix seem to cause trouble. On this one, I’m not sure what the intent was for using that for some imports but not others, so I thought I’d ask before making any big changes.


2. Install dependencies for the monorepo: `npm install`.

3. Install and link dependencies in sub-packages: `npx lerna bootstrap`
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 you’ve landed on leaving off hoist, is that intentional? I certainly think it’ll improve the top level dependency flow for now. But just wanted to check that this is actually the preferred flow.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yep, intentional! Seems to work just fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It’s a whole lot less fussy! I will say that I’m certain Yarn (1) would make hoisting work better for perf and a better FS impact. I can almost guarantee PNPM would too, just haven’t tried it. But this is definitely a case where perfection is something I’d avoid solving, I’m just happy to know I’m all good to install a top level dependency without wanting to take a smoke break during the re-bootrappening

@eyelidlessness
Copy link
Collaborator

Oof. So I'm back at my keyboard and giving this a go again, with a test importing microsite-build.ts like so:

import { suite } from 'uvu';
import * as assert from 'uvu/assert';
import build from '../../src/cli/microsite-build';


const component = suite('Build');

component('should render a basic HTML document', () => {
  assert.type(build, 'function');
});

component.run();

... which causes this error:

const require1 = createRequire(import.meta.url);
                                      ^^^^

SyntaxError: Cannot use 'import.meta' outside a module
    at wrapSafe (internal/modules/cjs/loader.js:915:16)
    at Module._compile (internal/modules/cjs/loader.js:963:27)
    at Module._compile (/Users/gnosis/Projects/eyelidlessness/microsite/node_modules/pirates/lib/index.js:99:24)
    at Module._extensions..js (internal/modules/cjs/loader.js:1027:10)
    at Object.newLoader [as .ts] (/Users/gnosis/Projects/eyelidlessness/microsite/node_modules/pirates/lib/index.js:104:7)
    at Module.load (internal/modules/cjs/loader.js:863:32)
    at Function.Module._load (internal/modules/cjs/loader.js:708:14)
    at Module.require (internal/modules/cjs/loader.js:887:19)
    at require (internal/modules/cjs/helpers.js:74:18)
    at Object.<anonymous> (/Users/gnosis/Projects/eyelidlessness/microsite/packages/microsite/test/cli/microsite-build.test.ts:3:1)

I think this is all being treated as CJS at runtime, which I fear may cause testing anything ESM-specific to be error prone and potentially untrustworthy.

@eyelidlessness
Copy link
Collaborator

I've also noticed that SWC is doing some weirdness with stuff in node_modules, it disallows a fair bit of syntax by default.

I think I might have one more go at the white whale. What do you think of this approach?

  1. A register wrapping esbuild's transform API, rather than build, directly (we basically have to trust it to reliably compile, it's at the core of much of the rest of the tooling).
  2. An experimental loader doing the same.
  3. Moving all -r flags to NODE_OPTIONS to ensure the test process is correctly run as ESM because Uvu assumes CJS when any -r flags are passed to it directly.

I'm out of steam for today but happy to look at this tomorrow if you think it's worth one more go.

@eyelidlessness
Copy link
Collaborator

Oh man, my thinking about this case had become very uptight!

It occurred to me when I mentioned that the other tooling uses esbuild... would it be absurd to just run tests through snowpack? I see there’s a first party test runner. It’s using @web/test-runner, which I’m not advocating for, but it suggests they consider test running a supported use case! Could even spin up an Uvu wrapper plugin or even a simple BYOR plugin, I know their plugin API doesn’t have a dedicated run API like rollup does, but there’s prior art to reference.

@eyelidlessness
Copy link
Collaborator

Obviously I haven’t fully run out of steam I’m just doing some research whilst sipping a whiskey on my porch. I haven’t read through all of this thread but it looks like it’ll be helpful!

@natemoo-re
Copy link
Owner Author

I'm not opposed to other approaches, but I'd ideally like to keep things as simple as possible. Seems like the tooling for Node esm is pretty immature at the moment, so we might have to do something custom-ish.

Using Snowpack would be interesting but Microsite can't really run in-browser at the moment, so there'd be some refactoring needed to make that happen (and I'm not sure it would even be possible.)

As for the .js suffix, yes that's a big sticking point. I want to remove all of the .js suffixes, have some step in the build process which adds them back. I'm working on building Microsite with esbuild so that will hopefully be a non-issue...

I wonder if shipping Microsite as dual .cjs and .mjs would kinda solve this whole thing? We'd drop the type: "module" from the package and we could keep everything in Node's default CJS land, while still shipping a compatible version for ESM node.

@eyelidlessness
Copy link
Collaborator

I'm not opposed to other approaches, but I'd ideally like to keep things as simple as possible.

On the same page! This is part of why I’m here working with this project! I actually think the snowpack solution might be the answer because it bakes in all the same assumptions the project already has. If it works out I’d almost certainly offer it as a separate dependency (like web/test-runner, with no opinion on which test framework is used; apparently that’s their design, the only difference is running on node instead of a browser; the discussion I linked goes into this and should be invaluable).

Using Snowpack would be interesting but Microsite can't really run in-browser at the moment, so there'd be some refactoring needed to make that happen (and I'm not sure it would even be possible.)

As for the .js suffix, yes that's a big sticking point. I want to remove all of the .js suffixes, have some step in the build process which adds them back. I'm working on building Microsite with esbuild so that will hopefully be a non-issue...

This is a big part of why the “use snowpack” idea felt like a big lightbulb. You already have a config that works for your project. A plugin which executes a test runner not only guarantees the least rework and weird branching logic, it also guarantees the tests are as close to the real world product as possible. I’ve seen enough of the snowpack and rollup plugin APIs to know you can just run anything you want on the build/dev product if you know where to plug in.

I wonder if shipping Microsite as dual .cjs and .mjs would kinda solve this whole thing? We'd drop the type: "module" from the package and we could keep everything in Node's default CJS land, while still shipping a compatible version for ESM node.

There’s some value in that for DX but I’m hesitant to endorse it. ESM is the future and that future is coming fast. Snowpack and rollup already have special benefits using ESM. Offering a CJS escape hatch is a really good way to deoptimize everything happening here. I’m not sure what the right answer is because like you said ESM is very immature where people are doing dev. But I think if I spend some time looking into this BYOR plugin it might shake some answers out.

@eyelidlessness
Copy link
Collaborator

Well, I tried the snowpack route first, and bailed pretty quickly because the perf hit at the installing dependencies stage was more than bad enough to negate any benefits.

I’m neck deep in trying to build out an experimental loader and esbuild is, well, complaining about perfectly valid TSX syntax. For instance here it complains about the { character, saying it expects >. In another case it complains about a ? character and doesn’t even point to a line/column number.

It’s possible I just haven’t found the right magic incantation for esbuild. But at this point I’m not sure what is actually going to work and I’m kind of antsy to get something working so I can build my site. If it’s okay with you, and if you’re comfortable with my proposed solution on #96, I may just suck it up and do that without a test and defer to you on test setup. Because I’m at a loss at this point.

@natemoo-re
Copy link
Owner Author

@eyelidlessness Yeah this is turning out to be a much bigger chore than expected. I'll take a look at #105 and we can circle back to this.

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.

None yet

2 participants