@W-21182296 Make 'webapplication.json' optional + validation#1682
@W-21182296 Make 'webapplication.json' optional + validation#1682Shinoni wants to merge 2 commits intoforcedotcom:mainfrom
Conversation
Shinoni
force-pushed
the
lt/web-app-descriptor-validation
branch
from
b1382cc to
c11198b
Compare
| } | ||
|
|
||
| if (!config.outputDir || typeof config.outputDir !== 'string') { | ||
| this.expectedSourceError(descriptorPath); |
There was a problem hiding this comment.
should these output the props that are missing?
There was a problem hiding this comment.
Nope. Pushed the fix that will output all missing props.
k-j-kim
left a comment
k-j-kim
left a comment
There was a problem hiding this comment.
other than a more specific error message for missing props, lgtm!
| } | ||
| if (this.tree.readFileSync(indexPath).length === 0) { | ||
| this.expectedSourceError(indexPath); | ||
| } |
There was a problem hiding this comment.
The errors messages (below) for validating the webapplication.json contents are really clear. These errors here too feel vague. How about something like: If a webapplication.json file does not exist, you are require to include a 'dist/index.tml' file. This will be the entry point for your webapplication
| } catch (e) { | ||
| const detail = e instanceof Error ? e.message : String(e); | ||
| throw new SfError(`Invalid JSON in webapplication.json: ${detail}`, 'InvalidJsonError'); | ||
| } |
There was a problem hiding this comment.
General question for the webapplication.json file and its contents: Do you have any public docs you could link to for the correct structure of this config file? This could help users validate all potential errors in a single pass rather than fixing one, getting an error, fixing one, etc.
There was a problem hiding this comment.
Not yet. We’re still working on finalizing its structure, but for now we’ve defined the required properties.
| ); | ||
| } | ||
|
|
||
| const fallbackPath = join(outputDirPath, config.routing.fallback.replace(/^\//, '')); |
There was a problem hiding this comment.
Should this replace use path.sep instead?
|
|
||
| const fallbackPath = join(outputDirPath, config.routing.fallback.replace(/^\//, '')); | ||
| if (!this.tree.exists(fallbackPath)) { | ||
| this.expectedSourceError(fallbackPath); |
There was a problem hiding this comment.
This also feels too vague. "The filepath defined in the webapplication.json -> routing.fallback was not found. Ensure this file exists at the location defined."
| if (Array.isArray(config.routing.rewrites)) { | ||
| for (const { rewrite } of config.routing.rewrites) { | ||
| if (rewrite) { | ||
| const rewritePath = join(outputDirPath, rewrite.replace(/^\//, '')); |
There was a problem hiding this comment.
path.sep instead of ^\/?
- webapplication.json optional: when absent or force-ignored, require non-empty dist/index.html - Validate required fields (outputDir, routing, trailingSlash, fallback) with clear error messages - Validate fallback and rewrite targets exist on disk - Precise error messages for dist fallback (missing folder, missing file, empty file) - Use path.sep for cross-platform path handling Co-authored-by: Cursor <cursoragent@cursor.com>
Shinoni
force-pushed
the
lt/web-app-descriptor-validation
branch
from
2b12667 to
68a8f79
Compare
…JSON test Co-authored-by: Cursor <cursoragent@cursor.com>
5 participants
What does this PR do?
Removed the requirement that webapplication.json must exist. If it exists, we check that the files it references are present. If it's missing or force-ignored, we require a non-empty dist/index.html instead. This supports apps without a JSON config and allows force-ignoring the descriptor.
What issues does this PR fix or reference?
N/A
#, @@
Functionality Before
webapplication.json was required, and at least one content file (besides the metadata XML) was required.
Functionality After
webapplication.json is optional; if present, we validate its fields and check that referenced files exist; if missing or force-ignored, we require a non-empty dist/index.html instead.