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

Replace Parcel with Rollup for manifest builder #389

Merged
merged 15 commits into from
Jul 8, 2020
Merged

Replace Parcel with Rollup for manifest builder #389

merged 15 commits into from
Jul 8, 2020

Conversation

pittst3r
Copy link
Contributor

@pittst3r pittst3r commented Jul 3, 2020

Motivation

I was initially attempting to upgrade to parcel@2 because:

  • Typechecking
  • Parcel errors if your app has parcel@2 installed while Bigtest is using parcel@1

After struggling with this I noticed:

  • We are having to do gymnastics in order to run Parcel (v1 and v2) in a safe way; specifically we are needing to run it in its own process so we can kill the whole group as it creates a bunch of child processes that don't shut down on their own
  • Neither the parcel@1 nor parcel@2 JS API are nice to use
  • We don't need the Parcel server feature, just a watched build

Approach

Rollup is a better fit for the problems at hand:

  • Has a first-class JS API
  • Does not require its own sandboxed process group

This PR switches to using Rollup for bundling the manifest.

To-do

Typechecking and linting of test code is still a problem. I was unable to get @rollup/plugin-typescript to behave well, so I fell back to using Babel to compile TypeScript, which does not do any typechecking. I think it may make sense to typecheck and lint as a separate step anyway if a tsconfig.json or .eslintrc.json are found.

@changeset-bot
Copy link

changeset-bot bot commented Jul 3, 2020

🦋 Changeset is good to go

Latest commit: a3d2bac

We got this.

This PR includes changesets to release 3 packages
Name Type
@bigtest/bundler Minor
@bigtest/server Minor
@bigtest/cli Patch

Not sure what this means? Click here to learn what changesets are.

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

@github-actions
Copy link
Contributor

github-actions bot commented Jul 3, 2020

The preview packages of this pull request have been published.
Click on the following packages for instructions on how to install them:

@bigtest/atom

Install using the following command:

$ npm install @bigtest/atom@rollup

Or update your package.json file:

{
  "@bigtest/atom": "rollup"
}

@bigtest/bundler

Install using the following command:

$ npm install @bigtest/bundler@rollup

Or update your package.json file:

{
  "@bigtest/bundler": "rollup"
}

@bigtest/effection

Install using the following command:

$ npm install @bigtest/effection@rollup

Or update your package.json file:

{
  "@bigtest/effection": "rollup"
}

@bigtest/globals

Install using the following command:

$ npm install @bigtest/globals@rollup

Or update your package.json file:

{
  "@bigtest/globals": "rollup"
}

@bigtest/interactor

Install using the following command:

$ npm install @bigtest/interactor@rollup

Or update your package.json file:

{
  "@bigtest/interactor": "rollup"
}

@bigtest/server

Install using the following command:

$ npm install @bigtest/server@rollup

Or update your package.json file:

{
  "@bigtest/server": "rollup"
}

Generated by 🚫 dangerJS against a3d2bac

@pittst3r pittst3r force-pushed the rollup branch 12 times, most recently from ab71bd2 to b162ba0 Compare July 6, 2020 13:40
@pittst3r pittst3r force-pushed the rollup branch 5 times, most recently from a413781 to 9bdef65 Compare July 6, 2020 21:34
@pittst3r pittst3r changed the title Rollup Replace Parcel with Rollup for manifest builder Jul 6, 2020
@pittst3r pittst3r marked this pull request as ready for review July 7, 2020 04:37
@pittst3r pittst3r requested review from jnicklas and cowboyd July 7, 2020 04:37
@jnicklas
Copy link
Collaborator

jnicklas commented Jul 7, 2020

@pittst3r :shipit: 🥳 🎈

Copy link
Member

@cowboyd cowboyd left a comment

Choose a reason for hiding this comment

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

This ought to simplify things considerably, way to turn a chore into an architectural upgrade!

packages/bundler/src/bundler.ts Outdated Show resolved Hide resolved
packages/bundler/src/bundler.ts Outdated Show resolved Hide resolved
packages/server/src/manifest-builder.ts Outdated Show resolved Hide resolved
);
let bundlerEvents: ChainableSubscription<BundlerMessage, undefined> = yield subscribe(bundler);
Copy link
Member

Choose a reason for hiding this comment

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

This is an interesting case of whether to use subscribe vs Subscribable.from. The first subtle difference is that the first maps an actual live subscription, whereas the latter maps an operation that produces a subscription.

From a runtime perspective, subscribe() produces a single subscription (tantamount to a single event listener) shared by both waitForSuccessfulBuild and createManifestBuilder vs two subscriptions, one at each site of iteration you would get from Subscribable.from. From a code perspective, subscribe() requires an extra yield statement and type annotation.

Otherwise, they're identical. I'm not sure I understand the tradeoffs fully, but wanted to at least highlight the differences for this use-case so that we can collect more datapoints. /cc @jnicklas

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One (fixable) problem with using Subscribable.from() is the Chain interface is not public, making type annotations with it impossible. This is not a great reason, but it was my deciding factor in this case. I do feel that subscribe() may be a better fit though as we would otherwise be recursively creating new subscriptions in waitForSuccessfulBuild() if I understand correctly.

As an aside, the overlap between Subscribable and Chain and ChainableSubscription is confusing to me. Not sure if there's room to make the differences more clear and understandable or otherwise consolidate into fewer interfaces.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I initially wanted to remove Subscribable entirely, but we left it in there, because we didn't know which type of chaining would turn out to be more useful, so we decided on keeping both. That being said, we could certainly improve on the naming of things to make it easier to understand.

Copy link
Member

Choose a reason for hiding this comment

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

@pittst3r seems like the right choice for a recursive method, although this is a very subtle distinction to ask users to

I wonder if we can make some shared interface that has all the chaining, and then have the low-level thing that just has next() on it a SubscriptionIterator. We might be able to use that chaining interface regardless of whether we're chaining the subscription or the operation.

@jnicklas jnicklas requested a review from cowboyd July 8, 2020 13:47
@pittst3r pittst3r merged commit ac5a7a6 into v0 Jul 8, 2020
@pittst3r pittst3r deleted the rollup branch July 8, 2020 16:36
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.

3 participants