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 self-link script as part of the pretest setup #14

Merged
merged 6 commits into from
Jan 17, 2019

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Nov 10, 2018

This should link (if needed) automatically before running tests

@haltcase haltcase added the type: chore General maintenance or upkeep. label Nov 12, 2018
@haltcase
Copy link
Owner

haltcase commented Nov 12, 2018

Thanks for this @Andarist! Unfortunately this is unix only and wouldn't work on Windows. Would you be willing to adapt this to a more portable solution? If not I'd like to at least document the differences in build steps between platforms.

package.json Show resolved Hide resolved
@Andarist
Copy link
Contributor Author

Thanks for this @Andarist! Unfortunately this is unix only and wouldn't work on Windows. Would you be willing to adapt this to a more portable solution? If not I'd like to at least document the differences in build steps between platforms.

I could probably try to unify it. Ideally I'd hide in a simple CLI tool using node APIs (I have no idea how to write powershell scripts, node APIs should take care of platform differences for me). Do you have an idea for the API of such a tool?

@haltcase
Copy link
Owner

haltcase commented Nov 14, 2018

how to write powershell scripts

I'm starting to use it a lot... it's awesome.

We can probably just do a small script at the root called .bootstrap.js, something like (untested):

const { existsSync } = require('fs')
const { join } = require('path')
const { execFileSync } = require('child_process')

const pkgPath = join('node_modules', process.env.npm_package_name)

if (!existsSync(pkgPath)) {
  execFileSync('npm', ['link'])
  execFileSync('npm', ['link', process.env.npm_package_name])
}

And then in package.json:

{
  "scripts": {
    "pretest": "node .bootstrap.js && npm run build",
    "test": "ava"
  }
}

@Andarist
Copy link
Contributor Author

I was worried about the fact that npm link runs a prepare scripts (so most likely build) and that would cause double build when running it for the first time - but I guess this is not really that much of a concern as it won't happen every time 🤔

Gonna prepare this thing some time later and adjust the PR!

@haltcase
Copy link
Owner

haltcase commented Nov 14, 2018

@Andarist great. Double build shouldn't be a problem but you could maybe make .bootstrap.js exit with an error code after linking, then use || or && somehow to prevent the npm run build step in the package script from triggering.

if (!existsSync(pkgPath)) {
  execFileSync('npm', ['link'])
  execFileSync('npm', ['link', process.env.npm_package_name])
  process.exitCode = 1
}

@Andarist
Copy link
Contributor Author

I was thinking about it (although werent sure if || is correct for powershell), but any exit code other than 0 would kinda mean some kind of error (as far as i know, not an expert in bash semantics though), so it wouldnt be quite correct to exit with 1 (or other number)

@haltcase
Copy link
Owner

I was thinking about it (although werent sure if || is correct for powershell)

It's not but npm scripts are run with cmd on Windows, not PowerShell, and both || and && would accomplish the same thing on Windows as on macOS/Linux.

any exit code other than 0 would kinda mean some kind of error (as far as i know, not an expert in bash semantics though), so it wouldnt be quite correct to exit with 1 (or other number)

¯\_(ツ)_/¯ an error code for what's not technically an error, yeah, but it'd do the trick for a wee script like this I think.

@Andarist Andarist force-pushed the patch-1 branch 2 times, most recently from 7731f2b to 786f5a1 Compare November 15, 2018 21:39
@Andarist
Copy link
Contributor Author

Andarist commented Nov 15, 2018

So I've created https://github.com/Andarist/npm-self-link as a simple tool to help with that task (I need to reuse it in 2 other macros). I've amended this PR to use it and I've invited u to collaboration on the tool (both on github & npm)

I see that you run your tests on node@6 too and I've used async/await syntax in the tool - gonna add a transpilation step some time later today to account for node@6.

@haltcase
Copy link
Owner

@Andarist sounds good! node 6 EoLs in April so I'd want to support it until then.

@Andarist Andarist force-pushed the patch-1 branch 7 times, most recently from 8d3be5b to 361eda3 Compare November 16, 2018 10:05
@Andarist
Copy link
Contributor Author

After a serious of fast broken releases 😅 I've added node@6 support to npm-self-link. Tests are passing here 😉

readme.md Outdated Show resolved Hide resolved
@haltcase haltcase merged commit f5ac85c into haltcase:master Jan 17, 2019
@haltcase
Copy link
Owner

Thanks @Andarist!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore General maintenance or upkeep.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants