Conversation
Split validation into schema, file checks, and Node-only file gathering. The default export is unchanged for Node consumers. Add a ./validatesubpath that takes (descriptor, fileList) so browser consumers can run full validation without Node APIs. - @adobe/reactor-validator/schema (schema validation only) - @adobe/reactor-validator/files (file validation with caller-provided file list).
…onsumers, be a little more restrictive with the viewBasePath.
…st case. 3.0.0-beta.5
| }; | ||
|
|
||
| var joinPath = function() { | ||
| return [].slice.call(arguments).filter(Boolean).join('/').replace(/\/+/g, '/'); |
There was a problem hiding this comment.
Should probably fix this to work for Windows as well.
gatherFilesInNodeEnvironment uses pathUtil.relative() which returns backslash-separated paths on windows. But, validateFiles.js constructs lookup paths using forward slashes via joinPath().
On windows, the fileSet would contain src\view\config.html but the validator would look for src/view/config.html causing false validation failures.
Looks like the old code avoided this because it used pathUtil.resolve() everywhere.
In gatherFilesInNodeEnvironment we could normalize separators to /...
result.push(relPath.replace(/\\/g, '/'));
...or normalize the file list in validateFiles.js when building the Set.
| if (!viewBasePath) return; | ||
|
|
||
| var viewBaseNorm = viewBasePath.replace(/\/+$/, ''); | ||
| if (fileSet.has(viewBaseNorm)) { |
There was a problem hiding this comment.
FYI - AI found a non-urgent edge case...
This checks if viewBaseNorm appears as an exact file in the set. If it does, it's assumed to be a file (not a directory). This is a reasonable heuristic for real-world extension packages. However, the redundant p === viewBaseNorm check at line 38 inside the some() can never succeed at that point (if the path were in the set as a file, we'd have already returned an error). Not harmful, but slightly confusing.
| return result; | ||
| }; | ||
|
|
||
| module.exports = gatherFilesInNodeEnvironment; |
There was a problem hiding this comment.
web-manifest.test.js and edge-manifest.test.js do cover this code, but there are no direct tests verifying:
- Recursive directory traversal
- Handling of symlinks, empty directories, or special characters in filenames
- Path separator output on different OS
Maybe consider if we need unit tests for these cases.
| var schemas = { | ||
| web: require('@adobe/reactor-turbine-schemas/schemas/extension-package-web.json'), | ||
| edge: require('@adobe/reactor-turbine-schemas/schemas/extension-package-edge.json'), | ||
| mobile: require('@adobe/reactor-turbine-schemas/schemas/extension-package-mobile.json') |
There was a problem hiding this comment.
Web and edge have test fixtures and tests, but I don't think mobile is used, right?
| "version": "3.0.0-beta.5", | ||
| "description": "Validates a Tags extension package.", | ||
| "main": "lib/index.js", | ||
| "exports": { |
There was a problem hiding this comment.
AI thoughts:
exports field may need a wildcard or additional entries
The exports field only exposes . and ./validate. If consumers were previously doing deep imports like require('@adobe/reactor-validator/lib/index'), those would now fail with ERR_PACKAGE_PATH_NOT_EXPORTED. This is appropriate for a semver major bump but worth documenting in migration notes.
2 participants
Description
Changes to allow validator to still be the "source of truth for validation" but can be used in a web context.
Related Issue
https://jira.corp.adobe.com/browse/PDCL-14485
Motivation and Context
This was a node-only project and wouldn't work due to it's inherent step of gathering files from the file system.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: