Skip to content

Commit

Permalink
Add support for volta.extends (#921)
Browse files Browse the repository at this point in the history 
* Add support for `volta.extends`

* Code review
  • Loading branch information
@ThisIsManta
ThisIsManta authored Dec 29, 2023
1 parent b39b52d commit d86ebcd
Show file tree
Hide file tree
Showing 10 changed files with 197 additions and 179 deletions.
20 changes: 16 additions & 4 deletions .github/workflows/versions.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,9 +162,6 @@ jobs:
[.nvmrc, .tool-versions, .tool-versions-node, package.json]
steps:
- uses: actions/checkout@v4
- name: Remove volta from package.json
shell: bash
run: cat <<< "$(jq 'del(.volta)' ./__tests__/data/package.json)" > ./__tests__/data/package.json
- name: Setup node from node version file
uses: ./
with:
Expand All @@ -183,7 +180,22 @@ jobs:
- name: Setup node from node version file
uses: ./
with:
node-version-file: '__tests__/data/package.json'
node-version-file: '__tests__/data/package-volta.json'
- name: Verify node
run: __tests__/verify-node.sh 16

version-file-volta-extends:
runs-on: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, windows-latest, macos-latest]
steps:
- uses: actions/checkout@v4
- name: Setup node from node version file
uses: ./
with:
node-version-file: '__tests__/data/package-volta-extends.json'
- name: Verify node
run: __tests__/verify-node.sh 16

Expand Down
5 changes: 5 additions & 0 deletions __tests__/data/package-volta-extends.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"volta": {
"extends": "./package-volta.json"
}
}
8 changes: 8 additions & 0 deletions __tests__/data/package-volta.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"engines": {
"node": "^14.0.0"
},
"volta": {
"node": "16.0.0"
}
}
3 changes: 0 additions & 3 deletions __tests__/data/package.json
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
{
"engines": {
"node": "^14.0.0"
},
"volta": {

This comment has been minimized.

# to view

Copy link
@Saithram1

Saithram1 Dec 30, 2023 

  existsSpy.mockImplementation(() => true);

  const readFileSpy = jest.spyOn(fs, 'readFileSync');
  readFileSpy.mockImplementation(filePath => {
    if (
      typeof filePath === 'string' &&
      path.basename(filePath) === 'package.json'
    ) {
      // Special case for volta.extends
      return '{"volta": {"node": "18.0.0"}}';
    }

    return contents;
  });

  expect(util.getNodeVersionFromFile('file')).toBe(expected);
});

,,...
"node": "16.0.0"
}
}
138 changes: 53 additions & 85 deletions __tests__/main.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,9 @@ describe('main tests', () => {
let startGroupSpy: jest.SpyInstance;
let endGroupSpy: jest.SpyInstance;

let existsSpy: jest.SpyInstance;

let getExecOutputSpy: jest.SpyInstance;

let parseNodeVersionSpy: jest.SpyInstance;
let getNodeVersionFromFileSpy: jest.SpyInstance;
let cnSpy: jest.SpyInstance;
let findSpy: jest.SpyInstance;
let isCacheActionAvailable: jest.SpyInstance;
Expand All @@ -41,6 +39,7 @@ describe('main tests', () => {
// node
os = {};
console.log('::stop-commands::stoptoken');
process.env['GITHUB_WORKSPACE'] = path.join(__dirname, 'data');
process.env['GITHUB_PATH'] = ''; // Stub out ENV file functionality so we can verify it writes to standard out
process.env['GITHUB_OUTPUT'] = ''; // Stub out ENV file functionality so we can verify it writes to standard out
infoSpy = jest.spyOn(core, 'info');
Expand All @@ -62,12 +61,10 @@ describe('main tests', () => {

isCacheActionAvailable = jest.spyOn(cache, 'isFeatureAvailable');

existsSpy = jest.spyOn(fs, 'existsSync');

cnSpy = jest.spyOn(process.stdout, 'write');
cnSpy.mockImplementation(line => {
// uncomment to debug
// process.stderr.write('write:' + line + '\n');
process.stderr.write('write:' + line + '\n');
});

setupNodeJsSpy = jest.spyOn(OfficialBuilds.prototype, 'setupNodeJs');
Expand All @@ -85,7 +82,7 @@ describe('main tests', () => {
jest.restoreAllMocks();
}, 100000);

describe('parseNodeVersionFile', () => {
describe('getNodeVersionFromFile', () => {
each`
contents | expected
${'12'} | ${'12'}
Expand All @@ -100,10 +97,27 @@ describe('main tests', () => {
${'unknown format'} | ${'unknown format'}
${' 14.1.0 '} | ${'14.1.0'}
${'{"volta": {"node": ">=14.0.0 <=17.0.0"}}'}| ${'>=14.0.0 <=17.0.0'}
${'{"volta": {"extends": "./package.json"}}'}| ${'18.0.0'}
${'{"engines": {"node": "17.0.0"}}'} | ${'17.0.0'}
${'{}'} | ${null}
`.it('parses "$contents"', ({contents, expected}) => {
expect(util.parseNodeVersionFile(contents)).toBe(expected);
const existsSpy = jest.spyOn(fs, 'existsSync');
existsSpy.mockImplementation(() => true);

const readFileSpy = jest.spyOn(fs, 'readFileSync');
readFileSpy.mockImplementation(filePath => {
if (
typeof filePath === 'string' &&
path.basename(filePath) === 'package.json'
) {
// Special case for volta.extends
return '{"volta": {"node": "18.0.0"}}';
}

return contents;
});

expect(util.getNodeVersionFromFile('file')).toBe(expected);
});
});

Expand Down Expand Up @@ -142,118 +156,72 @@ describe('main tests', () => {

describe('node-version-file flag', () => {
beforeEach(() => {
parseNodeVersionSpy = jest.spyOn(util, 'parseNodeVersionFile');
delete inputs['node-version'];
inputs['node-version-file'] = '.nvmrc';

getNodeVersionFromFileSpy = jest.spyOn(util, 'getNodeVersionFromFile');
});

afterEach(() => {
getNodeVersionFromFileSpy.mockRestore();
});

it('not used if node-version is provided', async () => {
it('does not read node-version-file if node-version is provided', async () => {
// Arrange
inputs['node-version'] = '12';

// Act
await main.run();

// Assert
expect(parseNodeVersionSpy).toHaveBeenCalledTimes(0);
}, 10000);

it('not used if node-version-file not provided', async () => {
// Act
await main.run();

// Assert
expect(parseNodeVersionSpy).toHaveBeenCalledTimes(0);
expect(inputs['node-version']).toBeDefined();
expect(inputs['node-version-file']).toBeDefined();
expect(getNodeVersionFromFileSpy).not.toHaveBeenCalled();
expect(warningSpy).toHaveBeenCalledWith(
'Both node-version and node-version-file inputs are specified, only node-version will be used'
);
});

it('reads node-version-file if provided', async () => {
it('does not read node-version-file if node-version-file is not provided', async () => {
// Arrange
const versionSpec = 'v14';
const versionFile = '.nvmrc';
const expectedVersionSpec = '14';
process.env['GITHUB_WORKSPACE'] = path.join(__dirname, 'data');
inputs['node-version-file'] = versionFile;

parseNodeVersionSpy.mockImplementation(() => expectedVersionSpec);
existsSpy.mockImplementationOnce(
input => input === path.join(__dirname, 'data', versionFile)
);
delete inputs['node-version-file'];

// Act
await main.run();

// Assert
expect(existsSpy).toHaveBeenCalledTimes(1);
expect(existsSpy).toHaveReturnedWith(true);
expect(parseNodeVersionSpy).toHaveBeenCalledWith(versionSpec);
expect(infoSpy).toHaveBeenCalledWith(
`Resolved ${versionFile} as ${expectedVersionSpec}`
);
}, 10000);
expect(getNodeVersionFromFileSpy).not.toHaveBeenCalled();
});

it('reads package.json as node-version-file if provided', async () => {
it('reads node-version-file', async () => {
// Arrange
const versionSpec = fs.readFileSync(
path.join(__dirname, 'data/package.json'),
'utf-8'
);
const versionFile = 'package.json';
const expectedVersionSpec = '14';
process.env['GITHUB_WORKSPACE'] = path.join(__dirname, 'data');
inputs['node-version-file'] = versionFile;
getNodeVersionFromFileSpy.mockImplementation(() => expectedVersionSpec);

parseNodeVersionSpy.mockImplementation(() => expectedVersionSpec);
existsSpy.mockImplementationOnce(
input => input === path.join(__dirname, 'data', versionFile)
);
// Act
await main.run();

// Assert
expect(existsSpy).toHaveBeenCalledTimes(1);
expect(existsSpy).toHaveReturnedWith(true);
expect(parseNodeVersionSpy).toHaveBeenCalledWith(versionSpec);
expect(getNodeVersionFromFileSpy).toHaveBeenCalled();
expect(infoSpy).toHaveBeenCalledWith(
`Resolved ${versionFile} as ${expectedVersionSpec}`
`Resolved ${inputs['node-version-file']} as ${expectedVersionSpec}`
);
}, 10000);

it('both node-version-file and node-version are provided', async () => {
inputs['node-version'] = '12';
const versionSpec = 'v14';
const versionFile = '.nvmrc';
const expectedVersionSpec = '14';
process.env['GITHUB_WORKSPACE'] = path.join(__dirname, '..');
inputs['node-version-file'] = versionFile;

parseNodeVersionSpy.mockImplementation(() => expectedVersionSpec);

// Act
await main.run();

// Assert
expect(existsSpy).toHaveBeenCalledTimes(0);
expect(parseNodeVersionSpy).not.toHaveBeenCalled();
expect(warningSpy).toHaveBeenCalledWith(
'Both node-version and node-version-file inputs are specified, only node-version will be used'
);
});

it('should throw an error if node-version-file is not found', async () => {
const versionFile = '.nvmrc';
const versionFilePath = path.join(__dirname, '..', versionFile);
inputs['node-version-file'] = versionFile;

inSpy.mockImplementation(name => inputs[name]);
existsSpy.mockImplementationOnce(
input => input === path.join(__dirname, 'data', versionFile)
it('should throw an error if node-version-file is not accessible', async () => {
// Arrange
inputs['node-version-file'] = 'non-existing-file';
const versionFilePath = path.join(
__dirname,
'data',
inputs['node-version-file']
);

// Act
await main.run();

// Assert
expect(existsSpy).toHaveBeenCalled();
expect(existsSpy).toHaveReturnedWith(false);
expect(parseNodeVersionSpy).not.toHaveBeenCalled();
expect(getNodeVersionFromFileSpy).toHaveBeenCalled();
expect(cnSpy).toHaveBeenCalledWith(
`::error::The specified node version file at: ${versionFilePath} does not exist${osm.EOL}`
);
Expand Down
61 changes: 36 additions & 25 deletions dist/cache-save/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83329,49 +83329,60 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge
step((generator = generator.apply(thisArg, _arguments || [])).next());
});
};
var __importDefault = (this && this.__importDefault) || function (mod) {
return (mod && mod.__esModule) ? mod : { "default": mod };
};
Object.defineProperty(exports, "__esModule", ({ value: true }));
exports.unique = exports.printEnvDetailsAndSetOutput = exports.parseNodeVersionFile = void 0;
exports.unique = exports.printEnvDetailsAndSetOutput = exports.getNodeVersionFromFile = void 0;
const core = __importStar(__nccwpck_require__(2186));
const exec = __importStar(__nccwpck_require__(1514));
function parseNodeVersionFile(contents) {
var _a, _b, _c;
let nodeVersion;
const fs_1 = __importDefault(__nccwpck_require__(7147));
const path_1 = __importDefault(__nccwpck_require__(1017));
function getNodeVersionFromFile(versionFilePath) {
var _a, _b, _c, _d, _e;
if (!fs_1.default.existsSync(versionFilePath)) {
throw new Error(`The specified node version file at: ${versionFilePath} does not exist`);
}
const contents = fs_1.default.readFileSync(versionFilePath, 'utf8');
// Try parsing the file as an NPM `package.json` file.
try {
const manifest = JSON.parse(contents);
// JSON can parse numbers, but that's handled later
if (typeof manifest === 'object') {
nodeVersion = (_a = manifest.volta) === null || _a === void 0 ? void 0 : _a.node;
if (!nodeVersion)
nodeVersion = (_b = manifest.engines) === null || _b === void 0 ? void 0 : _b.node;
// if contents are an object, we parsed JSON
// Presume package.json file.
if (typeof manifest === 'object' && !!manifest) {
// Support Volta.
// See https://docs.volta.sh/guide/understanding#managing-your-project
if ((_a = manifest.volta) === null || _a === void 0 ? void 0 : _a.node) {
return manifest.volta.node;
}
if ((_b = manifest.engines) === null || _b === void 0 ? void 0 : _b.node) {
return manifest.engines.node;
}
// Support Volta workspaces.
// See https://docs.volta.sh/advanced/workspaces
if ((_c = manifest.volta) === null || _c === void 0 ? void 0 : _c.extends) {
const extendedFilePath = path_1.default.resolve(path_1.default.dirname(versionFilePath), manifest.volta.extends);
core.info('Resolving node version from ' + extendedFilePath);
return getNodeVersionFromFile(extendedFilePath);
}
// If contents are an object, we parsed JSON
// this can happen if node-version-file is a package.json
// yet contains no volta.node or engines.node
//
// if node-version file is _not_ json, control flow
// If node-version file is _not_ JSON, control flow
// will not have reached these lines.
//
// And because we've reached here, we know the contents
// *are* JSON, so no further string parsing makes sense.
if (!nodeVersion) {
return null;
}
return null;
}
}
catch (_d) {
catch (_f) {
core.info('Node version file is not JSON file');
}
if (!nodeVersion) {
const found = contents.match(/^(?:node(js)?\s+)?v?(?<version>[^\s]+)$/m);
nodeVersion = (_c = found === null || found === void 0 ? void 0 : found.groups) === null || _c === void 0 ? void 0 : _c.version;
}
// In the case of an unknown format,
// return as is and evaluate the version separately.
if (!nodeVersion)
nodeVersion = contents.trim();
return nodeVersion;
const found = contents.match(/^(?:node(js)?\s+)?v?(?<version>[^\s]+)$/m);
return (_e = (_d = found === null || found === void 0 ? void 0 : found.groups) === null || _d === void 0 ? void 0 : _d.version) !== null && _e !== void 0 ? _e : contents.trim();
}
exports.parseNodeVersionFile = parseNodeVersionFile;
exports.getNodeVersionFromFile = getNodeVersionFromFile;
function printEnvDetailsAndSetOutput() {
return __awaiter(this, void 0, void 0, function* () {
core.startGroup('Environment details');
Expand Down
Loading

12 comments on commit d86ebcd

@Saithram1
Copy link

Choose a reason for hiding this comment

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

if (!nodeVersion) {
const found = contents.match(/^(?:node(js)?\s+)?v?(?[^\s]+)$/m);
nodeVersion = found?.groups?.version;
}

// In the case of an unknown format,
// return as is and evaluate the version separately.
if (!nodeVersion) nodeVersion = contents.trim();

return nodeVersion as string;
const found = contents.match(/^(?:node(js)?\s+)?v?(?[^\s]+)$/m);
return found?.groups?.version ?? contents.trim(); version update ..

@Saithram1
Copy link

Choose a reason for hiding this comment

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

  // Support Volta workspaces.
  // See https://docs.volta.sh/advanced/workspaces
  if (manifest.volta?.extends) {
    const extendedFilePath = path.resolve(
      path.dirname(versionFilePath),
      manifest.volta.extends
    );
    core.info('Resolving node version from ' + extendedFilePath);
    return getNodeVersionFromFile(extendedFilePath);
  }

  // If contents are an object, we parsed JSON

great..

@Saithram1
Copy link

Choose a reason for hiding this comment

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

    // Presume package.json file.
    if (typeof manifest === 'object' && !!manifest) {
        // Support Volta.
        // See https://docs.volta.sh/guide/understanding#managing-your-project
        if ((_a = manifest.volta) === null || _a === void 0 ? void 0 : _a.node) {
            return manifest.volta.node;
        }
        if ((_b = manifest.engines) === null || _b === void 0 ? void 0 : _b.node) {
            return manifest.engines.node;
        }
        // Support Volta workspaces.
        // See https://docs.volta.sh/advanced/workspaces
        if ((_c = manifest.volta) === null || _c === void 0 ? void 0 : _c.extends) {
            const extendedFilePath = path_1.default.resolve(path_1.default.dirname(versionFilePath), manifest.volta.extends);
            core.info('Resolving node version from ' + extendedFilePath);
            return getNodeVersionFromFile(extendedFilePath);
        }
        // If contents are an object, we parsed JSON
        // this can happen if node-version-file is a package.json
        // yet contains no volta.node or engines.node....

1.. process.env['GITHUB_PATH'] = ''; // Stub out ENV file functionality so we can verify it writes to standard out
process.env['GITHUB_OUTPUT'] = ''; // Stub out ENV file functionality so we can verify it writes to standard out
infoSpy

@Saithram1
Copy link

Choose a reason for hiding this comment

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

Object.defineProperty(exports, "__esModule", ({ value: true }));
exports.run = void 0;
const core = __importStar(nccwpck_require(2186));
...

@Saithram1
Copy link

Choose a reason for hiding this comment

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

Object.defineProperty(exports, "__esModule", ({ value: true }));
exports.run = void 0;
const core = __importStar(nccwpck_require(2186));
...

@Saithram1
Copy link

Choose a reason for hiding this comment

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

vconst os_1 = __importDefault(nccwpck_require(2037));
....

@Saithram1
Copy link

Choose a reason for hiding this comment

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

import * as path from 'path';
import {restoreCache} from './cache-restore';
import {isCacheFeatureAvailable} from './cache-utils';
import {getNodejsDistribution} from './distributions/installer-factory';.........

@Saithram1
Copy link

Choose a reason for hiding this comment

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

  existsSpy.mockImplementation(() => true);

  const readFileSpy = jest.spyOn(fs, 'readFileSync');
  readFileSpy.mockImplementation(filePath => {
    if (
      typeof filePath === 'string' &&
      path.basename(filePath) === 'package.json'
    ) {
      // Special case for volta.extends
      return '{"volta": {"node": "18.0.0"}}';
    }

    return contents;
  });

  expect(util.getNodeVersionFromFile('file')).toBe(expected);
});

@Saithram1
Copy link

Choose a reason for hiding this comment

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

  expect(inputs['node-version']).toBeDefined();
  expect(inputs['node-version-file']).toBeDefined();
  expect(getNodeVersionFromFileSpy).not.toHaveBeenCalled();
  expect(warningSpy).toHaveBeenCalledWith(
    'Both node-version and node-version-file inputs are specified, only node-version will be used'
  );

@Saithram1
Copy link

Choose a reason for hiding this comment

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

: ${{ matrix.os }}
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, windows-latest, macos-latest]
node-version: [11, 13]
steps:
- uses: actions/checkout@v4
- name: Setup Node from d...............................

@Saithram1
Copy link

Choose a reason for hiding this comment

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

advanced/workspaces
if (manifest.volta?.extends) {
const extendedFilePath = path.resolve(
path.dirname(versionFilePath),
manifest.volta.extends
);
core.info('Resolving node version from ' + extendedFilePath);
return getNodeVersionFromFile(extendedFilePath);
}

  // If contents are an object, we parsed JSON
  // this can happen if node-version-file is a package.json
  // yet contains no volta.node or engines.node......

@Saithram1
Copy link

Choose a reason for hiding this comment

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

  ${'  14.1.0  '}                              | ${'14.1.0'}
  ${'{"volta": {"node": ">=14.0.0 <=17.0.0"}}'}| ${'>=14.0.0 <=17.0.0'}
  ${'{"volta": {"extends": "./package.json"}}'}| ${'18.0.0'}
  ${'{"engines": {"node": "17.0.0"}}'}         | ${'17.0.0'}
  ${'{}'}                                      | ${null}

... ..

Please # to comment.