feat: encode minimal data in url search params and the rest in the url hash to allow for longer urls#819
Conversation
…arams to allow for longer urls while still maintaining server analytics feature
✅ Deploy Preview for livecodes ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Thank you @BassemHalim At first I was skeptical about this. However, after a quick check, it does work (see next comment) The query parameters are part of the public API for LiveCodes. I would not want to change that without providing a backward-compatible way to guarantee that the older URLs would keep working (with the maintenance overhead for that). See the failing tests. We do not need to change handling all query params. Only Also the newly added Let me think more about it. |
|
This does not work: while this works: |
|
This is just a proof of concept so it's fine if it doesn't get merged I was just tinkering :) In the last 2 links you provided i was able to load both but when I tried a longer file (~20KB) it didn't load for me before this fix. This is weird because I expected to get a 414 error from cloudflare instead of the infinite loading (or maybe I wasn't patient enough). So the limit seems to be in the browser (I tested with Brave) as well as the server. Anyway, hash url seems to fix that problem but I am curious why the browser would be able to handle a hash parameter and not a query parameter This didn't work while this works fine hashed url |
|
I'm getting more convinced with this. Instead of adding a new export const getParams = (
queryParams = parent.location.search,
hash = parent.location.hash,
): UrlQueryParams => {
let params: { [key: string]: string | boolean } = Object.fromEntries(
new URLSearchParams(queryParams),
);
if (hash) {
hash = hash.slice(1);
if (hash.startsWith('code/')) {
params.x = hash;
}
if (hash.startsWith('params/')) {
params.params = hash.slice('params/'.length);
}
}
let encodedParams = {};
Object.keys(params).forEach((key) => {
try {
const value = params[key] as string;
if (key === 'params') {
encodedParams = JSON.parse(decompress(value) || '{}');
if (!encodedParams || typeof encodedParams !== 'object')
encodedParams = {};
} else {
params[key] = decodeURIComponent(value);
}
} catch {
//
}
params = { ...encodedParams, ...params };
if (params[key] === '') params[key] = true;
if (params[key] === 'true') params[key] = true;
if (params[key] === 'false') params[key] = false;
});
return params;
};This overrides the params Then in share function, we change and in SDK we can change this to: url.hash = 'params/', compressToEncodedURIComponent(JSON.stringify(params));We are then left with These are still some thoughts. Let's see where this takes us. |
|
or even override them all, so we can store more than 1 value while defaulting to query params. if (hash) {
hash = hash.slice(1);
const hashParams = Object.fromEntries(new URLSearchParams(hash));
Object.keys(hashParams).forEach((key) => {
params[key] = hashParams[key];
});
} |
|
Cool! I like the idea of overriding params to maintain the backward compatibility. For the server analytics part, what kind of data do you need in the config? do you only need title and description? const serverContent = { title: content.title, description: content.description }
const contentParam = shortUrl
? '?x=id/' +
(await shareService.shareProject({
...content,
result: includeResult ? getCache().result : undefined,
}))
: '?x=code/' +compress(JSON.stringify(serverContent)) +'#x=code/' + compress(JSON.stringify(content));Because I saw in index.ts that that's the only data you are using or should i just clear the code and leave the rest of the metadata as is something like const serverConfig = {...content, markup: {}, script: {} etc.} // clear code fields (not exact just pseudocodeFinally, how do I test server functions like index.ts above? |
|
Currently we only need title and description for social cards. something like const params = new URLSearchParams();
if (content.title && content.title !== 'Untitled Project') {
params.set('title', encodeURIComponent(content.title));
}
if (content.description) {
params.set('description', encodeURIComponent(content.description));
}
const paramsString = params.toString() ? '?' + params.toString() : '';
const contentParam = shortUrl
? '?x=id/' +
(await shareService.shareProject({
...content,
result: includeResult ? getCache().result : undefined,
}))
: paramsString +'#x=code/' + compress(JSON.stringify(content));you can then receive this data here |
|
Hi @hatemhosny I implemented the following
Let me know what you think |
|
sample of the new verrrry long url ~25kB it loads just fine (sorry if it sends you a long email :) ) |
hatemhosny
left a comment
hatemhosny
left a comment
There was a problem hiding this comment.
Thank you @BassemHalim
I like how you construct the share URL 👍
I added a comment about overriding the params.
We need to reflect these changes in the SDK: here, here, here and maybe here (I can look into that last one)
please run npm run fix:prettier to fix the failing CI test.
Thank you :)
| ); | ||
| if (hashParams) { // overwrite params with hash params if they exist | ||
| hashParams = hashParams.replace('#', '?'); | ||
| params = Object.fromEntries(new URLSearchParams(hashParams)); |
There was a problem hiding this comment.
we should replace entries not the whole object.
what happens if we do this?
?console=open#x=code/...
I suggest this instead:
params = {
...params,
...Object.fromEntries(new URLSearchParams(hashParams)),
};… future search params
|
is it currently possible to share code between livecodes and the sdk? like some shared util functions? I was about to implement |
|
Yes, you can. I suggest making a new file (e.g utils.ts) in the sdk directory. Others can import from it. |
…fore setting hash or searchParams
hatemhosny
left a comment
hatemhosny
left a comment
There was a problem hiding this comment.
Thank you @BassemHalim
What started as a nice-to-have improvement in the share URL is now turning to be a significant refactor for how projects are loaded.
Honestly, I did not like the way it was. I think this will be a real improvement.
We just need to think of all cases and maintain backward compatibility.
I have added some comments (quite a lot, actually 😊). Please have a look and let me what you think.
Also after we settle on the implementation we need to add unit tests for getPlaygroundUrl. It has significant logic now. It also also a pure function that should be easy to test.
Thank you. I appreciate your effort and valuable suggestions.
| throw new Error(`"config" is not a valid URL or configuration object.`); | ||
| } | ||
| } else if (config && typeof config === 'object' && Object.keys(config).length > 0) { | ||
| playgroundUrl.searchParams.set('config', 'sdk'); // fixme: not sure of this one |
There was a problem hiding this comment.
This was intentional in createPlayground
The config object is then sent separately. See this
Obviously, we should not do that in getPlaygroundUrl.
It would be more appropriate to add it to hash params and remove the configHandler and the eventListener and also remove this.
This messaging was used in the first place to avoid passing the large object in the URL. This PR should solve this in a cleaner way.
There was a problem hiding this comment.
I removed the config:sdk params but haven't removed the event handlers yet
|
Hi @hatemhosny My current struggle so far was that some functionality was there to maintain backward comparability and it was a little hard finding the exact behavior of the function but I will ask you if I got stuck. |
|
Thank you @BassemHalim No worries at all. Your work has pushed me to think more about improvements I planed for quite some time. Yes, backward-compatibility is very important. We do not want to break our users code. Major breaking changes are always very frustrating. By the way, I have merged a PR (that also improves project loading logic), so please pull the new changes. |
… handle loading data from both hash and searchParams
|
Hi @hatemhosny I am also still testing to make sure i didn't introduce any bugs. |
| ): Promise<Partial<Config>> => { | ||
| //the url is config=code/... | ||
| const searchParams = new URLSearchParams(url); | ||
| if (searchParams.get('config') && isCompressedCode(searchParams.get('config') || '')) { |
There was a problem hiding this comment.
So I added this to import the code to handle loading from the url but I am not sure why the passed config object is not being used
There was a problem hiding this comment.
Let's not handle this here.
I prefer dealing with config like we deal with other external content, somewhere here. params.config is passed as configUrl. You may want to rename it to something like externalConfig, then check if it is a url to external config json or an encoded config object.
|
Hi @hatemhosny so I think I like getPlaygroundUrl now and I was thinking of making it the single source of truth for generating urls what do you think? We can also leave that to another PR so that we don't introduce too many changes so the const share = async (
shortUrl = false,
contentOnly = true,
urlUpdate = true,
includeResult = false,
permanentUrl = false,
): Promise<ShareData> => {
const config = getConfig();
const content = contentOnly
? {
...getContentConfig(config),
markup: {
...config.markup,
title: undefined,
hideTitle: undefined,
},
style: {
...config.style,
title: undefined,
hideTitle: undefined,
},
script: {
...config.script,
title: undefined,
hideTitle: undefined,
},
tools: {
...config.tools,
enabled: defaultConfig.tools.enabled,
status: config.tools.status === 'none' ? defaultConfig.tools.status : config.tools.status,
},
}
: config;
const currentUrl = (location.origin + location.pathname).split('/').slice(0, -1).join('/') + '/';
const appUrl = permanentUrl ? permanentUrlService.getAppUrl() : currentUrl;
let shareURL = new URL(appUrl);
if (shortUrl) {
shareURL.search =
'x=id/' +
(await shareService.shareProject({
...content,
result: includeResult ? getCache().result : undefined,
}));
} else {
const playgroundUrl = getPlaygroundUrl({ appUrl, config: content });
shareURL = new URL(playgroundUrl);
}
if (urlUpdate) {
updateUrl(shareURL.href, true);
}
const projectTitle = content.title !== defaultConfig.title ? content.title + ' - ' : '';
return {
title: projectTitle + 'LiveCodes',
url: shareURL.href,
};
};The url structure so far is short params in search params for server analytics and longer ones (config and params) in hash params |
There was a problem hiding this comment.
Thank you @BassemHalim
I like this.
I have added some comments, the most significant is how we deal with the encoded config object. Please let me know if you need more guidance.
so I think I like getPlaygroundUrl now and I was thinking of making it the single source of truth for generating urls what do you think?
I think this is reasonable. Please go ahead.
| ): Promise<Partial<Config>> => { | ||
| //the url is config=code/... | ||
| const searchParams = new URLSearchParams(url); | ||
| if (searchParams.get('config') && isCompressedCode(searchParams.get('config') || '')) { |
There was a problem hiding this comment.
Let's not handle this here.
I prefer dealing with config like we deal with other external content, somewhere here. params.config is passed as configUrl. You may want to rename it to something like externalConfig, then check if it is a url to external config json or an encoded config object.
| notifications.error( | ||
| window.deps.translateString('core.error.invalidImport', 'Invalid import URL'), | ||
| ); | ||
| } |
There was a problem hiding this comment.
So I moved handling the compressed config here but I had to move the "Invalid import URL" warning here because importCode() gets passed the hash parameter as url (I'm assuming this used to be a feature) and the function fails to import and returns an empty object. So now I updated the warning to be thrown if urlConfig is an empty object and configUrlConfig is undefined
There was a problem hiding this comment.
Indeed earlier on, I used the hash as the import url source then moved to the query param x (for social cards and analytics).
|
Thank you @BassemHalim Please wrap the last block in the SDK with this IIFE for tree-shaking, otherwise the whole file ends up in the bundle: /* @__PURE__ */ (() => {
// the code here
})();Sorting imports is beneficial, but please later on keep it in a separate commit because it gets difficult to see what changed. Please see if you want to add something else, othewise I think this is almost ready to merge. Thank you. |
|
Hi Dr. @hatemhosny I don't think I have anything to add to this PR at the moment but I am planing to open another PR later to refactor core.ts since it is too big. I will open an issue if you are open to the idea. |
Have a look here: https://esbuild.github.io/api/#pure Also try removing it and check the bundle size
No worries at all. It might be a good idea to automate it. Maybe something like this: https://www.npmjs.com/package/prettier-plugin-organize-imports
I'm open to improvements. Let's discuss that in an issue first. I will add a few things here before merge. I'd like to have your opinion. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
hi @BassemHalim Now the SDK and the app communicate their versions (even if self-hosted, regardless of the deployment URL) Demos: I also added CORS headers for netlify (for these demos to work!) Please have a look when you have time and let me know what you think. |
|
|
Added a note in docs about using multiple sources in embed options: https://deploy-preview-819--livecodes.netlify.app/docs/sdk/js-ts#multiple-sources |
|
I tested the newer version and everything is working perfectly. So from my understanding, now // for backward-compatibility
if (typeof config === 'object' && Object.keys(config).length > 0) {
playgroundUrl.searchParams.set('config', 'sdk');
}Also I a naive question but I am curious, when do old urls get retired? will the backward compatibility code always exist or maybe if a feature has been retired for x amount of time it gets dropped? I was thinking maybe we show a warning and update the browser url letting the user know to copy the new url. |
|
Thank you @BassemHalim for taking the time to test it. So the aim is to preserve backward compatibility for older SDK or older apps. Users can have old SDK (installed from npm or via a CDN URL) and no The other scenario is upgrading the SDK from npm while the So ideally, old URLs should keep working forever! (obviously as much as we can). At least now we setup a way of communication of versions between the SDK and the app, so that when the maintenance burden increases too much we can start showing a warning in the console to encourage upgrade. The SDK version is sent to the app in the The app version is sent by a That's why we have to keep sending However, the new app will not ask the SDK to send the config object (the old way), and the new SDK will not send it. Have I answered your questions? |
|
Another section in LiveCodes that relies on generating URLs to playgrounds is the embed screen Currently it generates a short share URL and uses it in the code snippets.
I prefer to refactor that to use the SDK in a better way, while still giving the option to use short URLs (only in our hosted platform). So now we have plans for 3 PRs:
|
|
That clarified it for me. Thank you! I don't currently have any suggestions for this PR Regarding the new PR plans, I think auto-sorting would be a short one so i can do that. Then, I can open an issue to discuss refactoring |
3 participants
What type of PR is this? (check all applicable)
Description
Currently, while browsers don't have a limit on url length some servers do implement a limit (for cloudflare it's 32KB )
so very long documents would not load. I tested this with a 20KB url and the page kept loading indefinitely.
I understand that some information is needed on the server side for analytics and for social media sharing. This PR implements the following, the share url would become something like:
base_url/?x=code/{compressed({title: "", description:""})#x=code/{compressed(config)}This helps maintain the same functionality on the server since we are using the same search params structure. However, I did not test the server analytics part yet.
This is currently a WIP. Feel free to close the PR if there is no need for this feature at the moment
This would allow sending minimal amount of data to the server while allowing for longer urls
Added tests?
Added to documentations?
[optional] Are there any post-deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?