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

E2E: Add Vue 2, add Angular, Mitosis output is source not package #500

Merged
merged 13 commits into from
Jun 22, 2022

Conversation

kylecordes
Copy link
Contributor

Revamp the E2E test to assume that Mitosis emits "source", not packaged output. This simplifies most of the harnesses, and much more closely matches the current capability of the project.

Add Vue 2 E2E.

Rename file outputs to enable further extension of the E2E.

@vercel
Copy link

vercel bot commented Jun 18, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
mitosis-fiddle ✅ Ready (Inspect) Visit Preview Jun 22, 2022 at 2:27AM (UTC)

@kylecordes
Copy link
Contributor Author

This seems like a big enough chunk to be worthy of merge.

Next step will be a way to have some tests fail and show the status somehow, rather than fail the build.

@kylecordes kylecordes marked this pull request as ready for review June 18, 2022 19:27
@kylecordes kylecordes requested review from a team and samijaber as code owners June 18, 2022 19:27
@kylecordes kylecordes changed the title E2E: consume Mitosis output as source. Test Vue 2. E2E: Add Vue 2, add Angular, Mitosis output is source not package Jun 19, 2022
@samijaber
Copy link
Contributor

@kylecordes with #497 merged, you should be able to point to vue2 and vue3 by just using the new targets I added to the CLI

@kylecordes
Copy link
Contributor Author

@samijaber Excellent! I will do that and also catch up with the merge conflicts.

Kyle Cordes added 2 commits June 21, 2022 20:58
@kylecordes
Copy link
Contributor Author

Vue 2 and 3 are now tested, both pass.

They also both accommodate one component using another; but that doesn't work in Angular yet, so it can't be in this simple single-example version of the E2E.

In the next PR I will work on accommodating partial failures to populate a "what works for what target" test status grid.

// 'qwik', // CLI does not support target: qwik
// 'builder', // CLI does not support target: builder
'html', // HTML output in a JS file
'webcomponent', // TS output in a JS file
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, we are now moving towards e2e-app simply storing our mitosis source code and not emitting anything? Can you elaborate a bit on why you preferred to sync the directory into each individual e2e app and run mitosis build there instead of building once in this location? Is the primary limitation not being able to provide a relative path for the destination? It sounds like your plan is to move back to the original system once the CLI becomes more powerful, so I want to have a clearer idea of what's blocking that.

Also, in this case, can we delete this mitosis.config.js file and the mitosis build command from e2e-app/package.json? Or are they still being used under some circumstances?

Copy link
Contributor Author

@kylecordes kylecordes Jun 22, 2022

Choose a reason for hiding this comment

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

In the former approach, the Mitosis app source code was compiled to all of the targets in the e2e-app. This initially seemed to work well but there were significant problems.

First, it meant that partial failure caused total failure. If mitosis had an error for any target, no other targets could be tested; we want to get away from that and toward partial success being reported in a test results grid.

Second and more importantly, upon study I found that Mitosis output, in the general case, is "source code", mostly, rather than npm-package-shaped. Of course it is more complex than that:

With React and Solid, there isn't much difference, and the Mitosis output would work as package output (with help for the package.json itself).

But with Vue 2, Vue 3, Angular, and (coming soon) Qwik, although it is sometimes possible with hackery to consume source code via node_modules, that is not the intended use, not the supported use from these libraries' point of view, and therefore not a good idea for our E2E. Instead, with these libraries the expectation is that a package will run its own compilation/bundling/etc process and publish the output as a npm package, which can then be consumed by another package. Mitosis does not yet have the machinery to do that for any of these targets, hence Mitosis output is “source”, not package.

(As I write this, I am unsure whether .svelte files are OK to consume directly from node_modules, or whether Svelte code is intended to be delivered in compiled form.)

For a point of reference, this describes the Angular Package Format; there are probably similar specs for Vue, and will likely be one for Qwik eventually. https://angular.io/guide/angular-package-format

For consistency and to support partial E2E success, with this new design all targets are treated as source code and copied over into the E2E harnesses to be compiled there.

I retained the mitosis.config.js and build command here though, firstly to work as a "smoke test" (so that we don't even attempt any E2E if the e2e-app is not a well-formed Mitosis input), and secondly to hold hope toward the possibility of Mitosis output becoming suitable for consumption as npm package contents in the future.

The src-directory-sync mechanism is annoying and should be unnecessary. I first tried an approach where Mitosis running in the E2E harnesses would directly look over at the source code in e2e-app, but found Mitosis doesn't accept this yet, and is internally hardcoded to only work with files: 'src'. #494

TLDR: this different design facilitates partial success (with more changes in that direction coming), and facilitates testing Mitosis as it work today.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the thoughtful write-up! I want to add as much context as possible based on our experience building Builder SDKs with Mitosis.

First, it meant that partial failure caused total failure. If mitosis had an error for any target, no other targets could be tested; we want to get away from that and toward partial success being reported in a test results grid.

Makes perfect sense 👍🏽

Instead, with these libraries the expectation is that a package will run its own compilation/bundling/etc process and publish the output as a npm package

I am not certain of how true that might be. For Vue and Svelte code, you don't need to run any additional compilation steps: you can provide an npm package that contains .svelte and .vue files and they will be consumed just fine.

In Svelte's case, it's actually problematic to pre-compile them before publishing, due to Svelte version mismatches. So you need to provide the original .svelte file: sveltejs/svelte#6584 (comment)

Our Mitosis-generated Builder SDKs are currently outputted in Vue, React-Native, SolidJS and Svelte, and all of them work out-of-the-box after mitosis build runs, without additional build steps (save for very few add-ons, like this solidjs index.jsx). All we need to do is provide the correct package.json exports/main/module/etc. fields for each output.

We have not yet outputted the SDK to Angular/Qwik, so what I'm saying might not apply to that, but I do want to point out that so far, our experience is that most of these modern frameworks (react-native, solid, svelte, vue2, vue3) do not need additional steps to work on top of mitosis build!

Curious if you've encountered any limitations/edge cases with Vue for e2e tests that might suggest otherwise.

Mitosis output is “source”

Agreed 💯! Mitosis output is fundamentally just source code for that particular output 👍🏽

Copy link
Contributor

Choose a reason for hiding this comment

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

secondly to hold hope toward the possibility of Mitosis output becoming suitable for consumption as npm package contents in the future.

I know we need to fix #494, and that can be done very easily soon.

One follow-up question is: what else is needed to get to that point? Sounds like Angular (& Qwik soon) will 100% need additional build logic, but that can be done on a case-by-case basis, such that yarn run build would call mitosis build && yarn run post-build:angular && yarn run post-build:qwik etc., right?

And going back to the partial failures, brainstorming some ideas here, we could:

  • provide a way to override the mitosis config with CLI flags and use that for --target, e.g. run each build separately in parallel, yarn run build --target=solid && yarn run build --target=vue2 etc.
  • add an ignore-errors flag with settings like skip-target (which skips the whole target if one file fails) and skip-file (which skips the file but keeps parsing the rest of the files for the same target) 🤔

Can add issues for these 2 outside this PR

Copy link
Contributor

@samijaber samijaber left a comment

Choose a reason for hiding this comment

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

Merging this since it's a net positive, discussed w/ @kylecordes and will have a followup PR accounting for the discussion in the comments (moving some source stuff back to being built in e2e-app)

@samijaber samijaber merged commit f977289 into BuilderIO:main Jun 22, 2022
@kylecordes kylecordes deleted the e2e-wip branch July 3, 2022 03:52
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.

2 participants