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

Improve dev script #468

Merged
merged 3 commits into from Apr 4, 2019
Merged

Improve dev script #468

merged 3 commits into from Apr 4, 2019

Conversation

mikkoh
Copy link
Contributor

@mikkoh mikkoh commented Apr 3, 2019

This PR does two things. Makes dev watch .ts files also and attempts to improve build speed for watch.

So I've seen #235 which is more extensive where it breaks out tests etc.

I'm putting this out there to bring this up as a discussion again.

Even after this PR build takes ~2sec to run which is not nice. I'm not actually sure why builds take so long. Might investigate at some point.

const path = require('path');
const rollup = require('rollup');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These weren't actually being used so I 🔥 them.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

};

const plugins = [
resolve(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed clean here in hopes that it would speed up builds.

}

warn(warning);
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I recognize this code could be put in it's module. I can do this in this PR if you all want.

Copy link
Contributor

@cdata cdata left a comment

Choose a reason for hiding this comment

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

Overall this looks like a good improvement. Would it be possible to make use of Rollup's --environment argument so that we don't need to use two separate Rollup config files?

const path = require('path');
const rollup = require('rollup');
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@mikkoh
Copy link
Contributor Author

mikkoh commented Apr 3, 2019

I'm cool with --environment if everyone else is cool with this. I believe #235 didn't use this.

@cdata
Copy link
Contributor

cdata commented Apr 3, 2019

It would make it easier to be specific about what we want in dev vs prod without sharing code and / or duplicating configuration. And, we could have more than just dev and prod scenarios (what about test?).

const resolve = require('rollup-plugin-node-resolve');
const cleanup = require('rollup-plugin-cleanup');

const {NODE_ENV} = process.env;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think using NODE_ENV has become a standard so 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@mikkoh
Copy link
Contributor Author

mikkoh commented Apr 4, 2019

I've updated to use environment instead of two distinct configs.

Copy link
Contributor

@cdata cdata left a comment

Choose a reason for hiding this comment

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

LGTM 👍 thanks for saving us previous dev time @mikkoh !

@@ -31,7 +31,8 @@
],
"scripts": {
"clean": "rm -rf ./lib ./dist",
"build": "tsc && rollup -c",
"build": "tsc && rollup -c --environment NODE_ENV:production",
"build:dev": "tsc && rollup -c --environment NODE_ENV:development",
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

const resolve = require('rollup-plugin-node-resolve');
const cleanup = require('rollup-plugin-cleanup');

const {NODE_ENV} = process.env;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@cdata cdata merged commit d42a3e1 into google:master Apr 4, 2019
@mrdoob
Copy link
Collaborator

mrdoob commented Apr 5, 2019

Thanks @mikkoh, my dev day today speed up considerably 🙏

@mikkoh
Copy link
Contributor Author

mikkoh commented Apr 5, 2019

🙌 I might look into it more and see if we can make some more speedups.

@cdata cdata added this to the v0.2.0 milestone Apr 5, 2019
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

3 participants