fix(size): encode dots with underscores instead of url encode #9
Conversation
ocombe
left a comment
ocombe
left a comment
There was a problem hiding this comment.
I don't see where you would fix errors like this one:
[error] Error: Reference.set failed: First argument contains an invalid key (bundle.br) in property 'payload.core.6%2E0%2Ex.484233f6c1d47dacde3a9111d0c79113bd2bb9ea.hello_world'. Keys must be non-empty strings and can't contain ".", "#", "$", "/", "[", or "]"
at /user_code/node_modules/firebase-admin/node_modules/@firebase/database/dist/index.node.cjs.js:1469:27
at Object.forEach (/user_code/node_modules/firebase-admin/node_modules/@firebase/database/node_modules/@firebase/util/dist/index.node.cjs.js:835:13)
at validateFirebaseData (/user_code/node_modules/firebase-admin/node_modules/@firebase/database/dist/index.node.cjs.js:1462:14)
at /user_code/node_modules/firebase-admin/node_modules/@firebase/database/dist/index.node.cjs.js:1479:13
at Object.forEach (/user_code/node_modules/firebase-admin/node_modules/@firebase/database/node_modules/@firebase/util/dist/index.node.cjs.js:835:13)
at validateFirebaseData (/user_code/node_modules/firebase-admin/node_modules/@firebase/database/dist/index.node.cjs.js:1462:14)
at validateFirebaseDataArg (/user_code/node_modules/firebase-admin/node_modules/@firebase/database/dist/index.node.cjs.js:1421:5)
at Reference.set (/user_code/node_modules/firebase-admin/node_modules/@firebase/database/dist/index.node.cjs.js:13963:9)
at SizeTask.<anonymous> (/user_code/dist/plugins/size.js:128:31)
at next (native)
at /user_code/dist/plugins/size.js:7:71
at __awaiter (/user_code/dist/plugins/size.js:3:12)
at SizeTask.upsertNewArtifacts (/user_code/dist/plugins/size.js:95:16)
at SizeTask.<anonymous> (/user_code/dist/plugins/size.js:81:25)
at next (native)
at fulfilled (/user_code/dist/plugins/size.js:4:58)
at process._tickDomainCallback (internal/process/next_tick.js:135:7)have you checked the script scripts/ci/payload-size.sh to be sure that we don't need to encode anything else?
functions/src/plugins/size.ts
Outdated
|
|
||
| getRef(path: string): database.Reference { | ||
| return this.rtDb.ref(firebasePathEncode(path)); | ||
| return this.rtDb.ref(path); |
There was a problem hiding this comment.
if you don't encode the path in this function, then it has no use, you might as well remove it
functions/src/plugins/size.ts
Outdated
| for(const project of projects) { | ||
| for(const branch of context.payload.branches) { | ||
| const ref = this.getRef(`/payload/${project}/${branch.name}/${context.payload.commit.sha}`); | ||
| const ref = this.getRef(`/payload/${project}/${firebasePathEncode(branch.name)}/${context.payload.commit.sha}`); |
There was a problem hiding this comment.
are we sure that we only want to encode the branch name?
There was a problem hiding this comment.
https://github.com/angular/angular/blob/master/scripts/ci/payload-size.sh#L88
looks like the only part that needs encoding
| */ | ||
| export function firebasePathEncode(s: string) { | ||
| return s.replace(/[\.#\$\[\]]/g, c => '%' + c.charCodeAt(0).toString(16).toUpperCase()); | ||
| return s.replace(/\./g, '_'); |
There was a problem hiding this comment.
shouldn't we also replace other forbidden characters?
There was a problem hiding this comment.
Going off the existing script I don't think so. I think there's other cases where it's needed it should be handled on a case by case basis
| let lastNestedItemRef: object | number = artifactsOutput; | ||
| // first item is the project name which we've used already | ||
| a.contextPath.forEach((path, i) => { | ||
| const encodedPath = firebasePathEncode(path); |
There was a problem hiding this comment.
@ocombe The changes here are the ones that are intended to fix the bundle.br issue
| Object.keys(object).forEach(k => { | ||
| if(typeof object[k] === 'object') { | ||
| reconstructArtifacts(object[k], path + '/' + k); | ||
| reconstructArtifacts(object[k], path + '/' + firebasePathDecode(k)); |
There was a problem hiding this comment.
how can you differentiate between existing "_" from the name, and the one that you added when you did the encode?
for example if the name was "bundle_2.br", it would be encoded into "bundle_2_br" and decoded into "bundle.2.br"
is it a problem?
There was a problem hiding this comment.
bugger.... You're right.
I don't know how you'd get around that, from what I can decipher there's isn't any special handling for this case in the payload-size.sh
2 participants
No description provided.