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

Manifest builder improvements #401

Merged
merged 11 commits into from
Jul 14, 2020
Merged

Manifest builder improvements #401

merged 11 commits into from
Jul 14, 2020

Conversation

pittst3r
Copy link
Contributor

@pittst3r pittst3r commented Jul 12, 2020

Motivation

I noticed a few things to clean up:

  • Rollup wasn't properly excluding node_modules from watch
  • The Bundler try/finally was not wrapping the code that needed it
  • At the default log level, users see manifest build errors, but nothing when the build becomes successful
  • There is a bug in fs.promises.truncate() that causes warnings fsPromises.truncate doesn't close fd. nodejs/node#34189

Approach

  • Use a regex to exclude node_modules from rollup watch
  • Use ensure() API instead of try/finally in Bundler
  • Upgrade the log level of the successful manifest build notification
  • Use ftruncate() instead of truncate() until the fix is released

@changeset-bot
Copy link

changeset-bot bot commented Jul 12, 2020

🦋 Changeset is good to go

Latest commit: 5e0adf7

We got this.

This PR includes changesets to release 2 packages
Name Type
@bigtest/bundler Patch
@bigtest/server 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 12, 2020

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

@bigtest/agent

Install using the following command:

$ npm install @bigtest/agent@manifest

Or update your package.json file:

{
  "@bigtest/agent": "manifest"
}

@bigtest/atom

Install using the following command:

$ npm install @bigtest/atom@manifest

Or update your package.json file:

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

@bigtest/bundler

Install using the following command:

$ npm install @bigtest/bundler@manifest

Or update your package.json file:

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

@bigtest/cli

Install using the following command:

$ npm install @bigtest/cli@manifest

Or update your package.json file:

{
  "@bigtest/cli": "manifest"
}

@bigtest/interactor

Install using the following command:

$ npm install @bigtest/interactor@manifest

Or update your package.json file:

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

@bigtest/server

Install using the following command:

$ npm install @bigtest/server@manifest

Or update your package.json file:

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

@bigtest/webdriver

Install using the following command:

$ npm install @bigtest/webdriver@manifest

Or update your package.json file:

{
  "@bigtest/webdriver": "manifest"
}

Generated by 🚫 dangerJS against 5e0adf7

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.

Nice set of stability fixes! 🍷

packages/server/src/manifest-builder.ts Outdated Show resolved Hide resolved
packages/server/src/manifest-builder.ts Outdated Show resolved Hide resolved
});

rollup = watch(prepareRollupOptions(bundles));
let events: ChainableSubscription<Array<RollupWatcherEvent>, undefined> = yield subscribe(on<Array<RollupWatcherEvent>>(rollup, 'event'));
Copy link
Member

Choose a reason for hiding this comment

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

I'm still not quite sure why a try/catch won't catch the right thing. If you put it around 82-90, it will catch any exception raised therein.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what it was originally:

let rollup: RollupWatcher = watch(prepareRollupOptions(bundles));;
let events: ChainableSubscription<Array<RollupWatcherEvent>, undefined> = yield subscribe(on<Array<RollupWatcherEvent>>(rollup, 'event'));
let messages = events
  .map(([event]) => event)
  .filter(event => event.code === 'END' || event.code === 'ERROR')
  .map(event => event.code === 'ERROR' ? { type: 'error', error: event.error } : { type: 'update' });

try {
  yield messages.forEach(function*(message) {
    bundler.channel.send(message as BundlerMessage);
  });
} finally {
  rollup.close();
}

Which is wrong I think, prompting me to make a change. However, rather than just wrap more in the try block I thought ensure() was the way to go, but now that you've explained that that's not the way to go I'll update to this:

let rollup: RollupWatcher = watch(prepareRollupOptions(bundles));;

try {
  let events: ChainableSubscription<Array<RollupWatcherEvent>, undefined> = yield subscribe(on<Array<RollupWatcherEvent>>(rollup, 'event'));
  let messages = events
    .map(([event]) => event)
    .filter(event => event.code === 'END' || event.code === 'ERROR')
    .map(event => event.code === 'ERROR' ? { type: 'error', error: event.error } : { type: 'update' });

  yield messages.forEach(function*(message) {
    bundler.channel.send(message as BundlerMessage);
  });
} finally {
  rollup.close();
}

@pittst3r pittst3r merged commit 20de888 into v0 Jul 14, 2020
@pittst3r pittst3r deleted the manifest branch July 14, 2020 14: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.

2 participants