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

Add bundle tasks for distribution #588

Merged
merged 4 commits into from
Feb 3, 2017
Merged

Add bundle tasks for distribution #588

merged 4 commits into from
Feb 3, 2017

Conversation

giladgray
Copy link
Contributor

@giladgray giladgray commented Feb 1, 2017

Fixes #303

Changes proposed in this pull request:

new typescript-bundle task group webpacks each package's compiled js files and spits out dist/[name].bundle.js.

Reviewers should focus on:

  • is there a convention for specifying a bundled file in package.json? a la main/typings/style? i couldn't find anything relevant after much googling.

Gilad Gray added 2 commits January 31, 2017 18:24
spits out `dist/[name].bundle.js` for each package.
@blueprint-bot
Copy link

dangit bad javascript

Preview: docs
Coverage: core | datetime

@adidahiya
Copy link
Contributor

@giladgray listed in the original ticket. The field is called "browser" https://github.com/defunctzombie/package-browser-field-spec

@@ -53,4 +55,17 @@ module.exports = (gulp, plugins, blueprint) => {
tsResult.dts,
]).pipe(blueprint.dest(project));
});

const bundleTaskNames = blueprint.projectsWithBlock("typescript").map((project) => {
const taskName = `typescript-bundle-${project.id}`;
Copy link
Contributor

@adidahiya adidahiya Feb 1, 2017

Choose a reason for hiding this comment

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

this doesn't really have anything to do with typescript -- it should just be called bundle-${project.id} or bundle-dist-${project.id}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

soooo i had a similar thought, but it does have to do with the typescript config block, which is the naming convention for compile tasks: block-name-project. i can't use blueprint.task() directly here to enforce that pattern because that method doesn't support callback task definitions, so i had to spell it out by hand.

that said, i don't feel too strongly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adidahiya ping on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind spelling it out by hand rather than using blueprint.task(); I just think the task name is wrong and it shouldn't be in the gulp/typescript.js file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and i'm disagreeing with both of those because it has everything to do with the typescript sources

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really, because generateWebpackBundleConfig uses dist/index.js as an entry point (not a TS file). This is the whole deal with splitting up our build process into a tsc stage and a webpack stage. We can follow up further offline about this if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok sure i hear that. let's discuss tomorrow, i'm feeling build-y 🔨

Copy link
Contributor

Choose a reason for hiding this comment

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

offline resolution: move this to gulp/dist.js

@adidahiya adidahiya changed the title [name].bundle.js Add webpack-dist gulp task Feb 2, 2017
@adidahiya adidahiya changed the title Add webpack-dist gulp task Add dist-bundle gulp task Feb 2, 2017
@adidahiya adidahiya changed the title Add dist-bundle gulp task Add bundle tasks for distribution Feb 3, 2017
@blueprint-bot
Copy link

move to dist/, call it "bundle"

Preview: docs | landing | table
Coverage: core | datetime

@giladgray
Copy link
Contributor Author

📦 core.bundle.js

@blueprint-bot
Copy link

remove unused deps (forgot to save)

Preview: docs | landing | table
Coverage: core | datetime

@adidahiya
Copy link
Contributor

That is a big bundle. Filed a follow-up #612 :)

@giladgray giladgray merged commit 82d2f06 into master Feb 3, 2017
@giladgray giladgray deleted the gg/bundle-task branch February 3, 2017 01:27
@giladgray giladgray mentioned this pull request Feb 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants