Skip to content

Restructure builds #76

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

Merged
merged 2 commits into from
Oct 17, 2015
Merged

Restructure builds #76

merged 2 commits into from
Oct 17, 2015

Conversation

jbrantly
Copy link
Member

@jbrantly jbrantly commented Oct 6, 2015

  • Add TypeScript as dev dependency
  • Use npm scripts instead of node build. Update docs.
  • Move build.js to separate folder to declutter root

Meant to resolve this comment

cc @blakeembrey

- Add TypeScript as dev dependency
- Use npm scripts instead of `node build`. Update docs.
- Move build.js to separate folder to declutter root
@blakeembrey
Copy link
Member

LGTM 👍

Side note: Why is there two different build paths? Why not just move that into the scripts directly?

@jbrantly
Copy link
Member Author

jbrantly commented Oct 6, 2015

Why is there two different build paths?

Do you mean why is there this code?

if (semver.lt(typescript.version, '1.6.0-0')) {
    exec('tsc index.ts --module commonjs', cb)
}
else {
    exec('tsc index.ts --module commonjs --moduleResolution classic', cb)
}

It's because right now I have to use the classic module resolution, so I need to specify that. However, if I specify that for TS 1.5 I get an error back saying unknown option. Once we ditch support for TS 1.5 the build.js script can go away completely.

Why not just move that into the scripts directly?

Not sure I follow.

@blakeembrey
Copy link
Member

You could use "build": "tsc index.ts --module commonjs --moduleResolution classic".

If I'm reading this correct (below), build would be running after the initial install with the correct devDependency version we support anyway. Then we install the TypeScript version we test with.

- npm install
- npm install $TYPESCRIPT

@@ -4,9 +4,10 @@
"description": "TypeScript loader for webpack",
"main": "index.js",
"scripts": {
"pretest": "node build",
"build": "node scripts/build",
"pretest": "npm run build",
"test": "node ./node_modules/mocha/bin/mocha --reporter spec test/run.js",
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated, but thoughts on code coverage? Also, this line could be just mocha --reporter spec test/run.js

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't really given much thought to code coverage, but 👍 if you want to implement it :D

@jbrantly
Copy link
Member Author

jbrantly commented Oct 6, 2015

build would be running after the initial install with the correct devDependency version we support

That's true, but we'd lose build support when actively developing for TS 1.5. Meaning...

npm install [email protected]
npm run build

would fail. But given that I haven't made any 1.5.3-specific changes in a while and I do want to eventually drop support for it, I would be OK with this. At the time that I implemented this 1.6.0-beta wasn't even released yet so keeping 1.5.3 support was more important.

@jbrantly
Copy link
Member Author

jbrantly commented Oct 6, 2015

One other issue with dropping build.js. In the travis build, npm test will actually rebuild the JS due to the "pretest" script. Should we just drop the pretest?

@blakeembrey
Copy link
Member

Yeah, drop pretest. It's already built. The version we build against we can configure anytime, the version we run is definitely less configurable.

@jbrantly
Copy link
Member Author

@blakeembrey I integrated your suggestions so I'm going to go ahead and merge. Thanks!

jbrantly added a commit that referenced this pull request Oct 17, 2015
@jbrantly jbrantly merged commit 7f79b34 into master Oct 17, 2015
@johnnyreilly johnnyreilly deleted the update-build branch July 13, 2017 15:29
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