-
Notifications
You must be signed in to change notification settings - Fork 592
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
support A3 projects with npm workspaces packages #4284
Changes from all commits
83d5884
4d66e0d
24b2d9c
daeb8e2
b35a172
133274f
591aaa0
2296412
446091a
1145cb3
30c0ae6
6b3d9c1
6b3d1a5
6f5972c
5d7d661
131c9c3
48708bd
0abdbde
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -186,10 +186,13 @@ module.exports = function(options) { | |
const initialFolder = path.dirname(self.root.filename); | ||
let folder = initialFolder; | ||
while (true) { | ||
const file = `${folder}/package.json`; | ||
const file = path.resolve(folder, 'package.json'); | ||
if (fs.existsSync(file)) { | ||
const info = JSON.parse(fs.readFileSync(file, 'utf8')); | ||
self.validPackages = new Set([ ...Object.keys(info.dependencies || {}), ...Object.keys(info.devDependencies || {}) ]); | ||
self.validPackages = new Set([ | ||
...getDependencies(info), | ||
...getWorkspacesDependencies(folder)(info) | ||
]); | ||
break; | ||
} else { | ||
folder = path.dirname(folder); | ||
|
@@ -231,7 +234,36 @@ module.exports = function(options) { | |
} | ||
self.symlinksCache.set(type, link); | ||
return link; | ||
}; | ||
} | ||
|
||
function getDependencies({ | ||
dependencies = {}, | ||
devDependencies = {} | ||
} = {}) { | ||
return [ | ||
...Object.keys(dependencies || {}), | ||
...Object.keys(devDependencies || {}) | ||
]; | ||
} | ||
|
||
function getWorkspacesDependencies(folder) { | ||
return ({ workspaces = [] } = {}) => { | ||
if (workspaces.length === 0) { | ||
return []; | ||
} | ||
|
||
// Ternary is required because glob expects at least 2 entries when using curly braces | ||
const pattern = workspaces.length === 1 ? workspaces[0] : `{${workspaces.join(',')}}`; | ||
const packagePath = path.resolve(folder, pattern, 'package.json'); | ||
const workspacePackages = glob.sync(packagePath, { follow: true }); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. niiice There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Very slick use of advanced glob features |
||
|
||
return workspacePackages.flatMap(packagePath => { | ||
const info = JSON.parse(fs.readFileSync(packagePath, 'utf8')); | ||
|
||
return getDependencies(info); | ||
}); | ||
}; | ||
} | ||
|
||
self.isImprovement = function(name) { | ||
return _.has(self.improvements, name); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
const assert = require('node:assert').strict; | ||
const util = require('node:util'); | ||
const { exec } = require('node:child_process'); | ||
const path = require('node:path'); | ||
const t = require('../test-lib/test.js'); | ||
const app = require('./workspaces-project/app.js'); | ||
|
||
describe('workspaces dependencies', function() { | ||
this.timeout(t.timeout); | ||
|
||
before(async function() { | ||
await util.promisify(exec)('npm install', { cwd: path.resolve(process.cwd(), 'test/workspaces-project') }); | ||
}); | ||
|
||
it('should allow workspaces dependency in the project', async function() { | ||
let apos; | ||
|
||
try { | ||
apos = await t.create(app); | ||
const { server } = apos.modules['@apostrophecms/express']; | ||
const { address, port } = server.address(); | ||
|
||
const actual = apos.util.logger.getMessages(); | ||
const expected = { | ||
debug: [], | ||
info: [ `Listening at http://${address}:${port}` ], | ||
warn: [], | ||
error: [] | ||
}; | ||
|
||
assert.deepEqual(actual, expected); | ||
} catch (error) { | ||
assert.fail('Should have found @apostrophecms/sitemap hidden in workspace-a as a valid dependency. '.concat(error.message)); | ||
} finally { | ||
apos && await t.destroy(apos); | ||
} | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
module.exports = { | ||
root: module, | ||
shortName: 'workspaces-project', | ||
modules: { | ||
'@apostrophecms/express': { | ||
options: { | ||
address: '127.0.0.1' | ||
} | ||
}, | ||
'@apostrophecms/sitemap': { | ||
options: { | ||
baseUrl: 'http://localhost:3000' | ||
} | ||
} | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
const createLogger = () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good technique. |
||
const messages = { | ||
debug: [], | ||
info: [], | ||
warn: [], | ||
error: [] | ||
}; | ||
|
||
return { | ||
debug: (...args) => { | ||
console.debug(...args); | ||
messages.debug.push(...args); | ||
}, | ||
info: (...args) => { | ||
console.info(...args); | ||
messages.info.push(...args); | ||
}, | ||
warn: (...args) => { | ||
console.warn(...args); | ||
messages.warn.push(...args); | ||
}, | ||
error: (...args) => { | ||
console.error(...args); | ||
messages.error.push(...args); | ||
}, | ||
destroy: () => { | ||
delete messages.debug; | ||
delete messages.info; | ||
delete messages.warn; | ||
delete messages.error; | ||
}, | ||
getMessages: () => messages | ||
}; | ||
}; | ||
|
||
module.exports = { | ||
options: { | ||
logger: createLogger() | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
{ | ||
"name": "workspace-project", | ||
"version": "1.0.0", | ||
"description": "", | ||
"main": "app.js", | ||
"scripts": { | ||
"test": "echo \"Error: no test specified\" && exit 1" | ||
}, | ||
"keywords": [], | ||
"author": "", | ||
"license": "ISC", | ||
"dependencies": { | ||
"apostrophe": "file:../../." | ||
}, | ||
"workspaces": [ | ||
"workspace-a" | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
{ | ||
"name": "workspace-a", | ||
"version": "1.0.0", | ||
"description": "", | ||
"main": "index.js", | ||
"scripts": { | ||
"test": "echo \"Error: no test specified\" && exit 1" | ||
}, | ||
"keywords": [], | ||
"author": "", | ||
"license": "ISC", | ||
"dependencies": { | ||
"@apostrophecms/sitemap": "^1.0.2" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure the join call is always sufficient here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a comment, glob expects at least two entries when using curly braces. Silly bug!