-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
VYZN: Support for HTTPS data provider #719
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for bldrs-share ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@pablo-mayrgundter Before we write tests, is this draft OK? |
Huh! Really interesting. Yeah, probably fine. Why https only? Also, please provide a working demo URL in the PR's description. |
@Adrian62D ping |
@pablo-mayrgundter Zien is is progress of coding automated tests for this PR. Updated description and http will follow. |
Hi @pablo-mayrgundter @Adrian62D For example the "Momentum" file link would be NOTE: Github hosted files already allows all origins however sentry adds two headers (baggage, sentry trace) that aren't allowed, which causes the request to be CORS blocked (we might want to change that in sentry settings) These are two examples: |
@oo-bldrs Can I tag you in on this.. CORS stuff I don't understand... |
From my reading of this code, the logic is to download the URL from the remote source into memory, turn it into a blob, then pass in a That seems fairly inefficient by creating multiple copies of a potentially large remote object. Is there a blocker that explains why we cannot pass the URL along to the loader and let it directly access the content? |
The internal model of bldrs distinguishes between local files and github files. There is currently no abstraction layer that lets you plug in additional data sources in an optimized fashion. A remote file from an http/https source can not fulfil the contract of a github file, therefore it is being passes to bldrs as a local file in this PR. Our recommendation is to introduce an abstraction layer at some point in time in the future. |
Hey guys, just talked with @oo-bldrs. Thanks Ogali for a very detailed walkthrough. In summary we think the CORS issues might be a red herring. CORS sentry headers aren't causing issues here.. gh blocking all CORS requests. Same for the aws buckets, unless their owner configures differently. Your use of a proxy makes sense to get around that, but is out of scope for us. Ogali and I are available to discuss more in a discord thread: I'll take the PR back to just focus on the new src loader handling. |
Hi @aozien, per suggestion from Ogali, I'm wondering if the src URL loading can be delegated to the loader later in CadView, to avoid the performance hit esp for larger models. Did you consider that and reject it for any reason? I can have a look to see if it's obvious to me how to do it as well. |
@pablo-mayrgundter The decision was made by me. I tried to summarize the reasoning here -> #719 (comment) |
2e5d3bc
to
a72d555
Compare
@Adrian62D @pablo-mayrgundter is this still relevant |
@OlegMoshkovich Yes |
@pablo-mayrgundter shall this PR remain open? |
@pablo-mayrgundter, this is an unacceptable UX and creates a bad impression of the product. From my point of view, it should be fixed as soon as possible, especially if we plan to expose the 'Share' to Nokera. |
This is a draft.
Implementation that should close #647.
The existing Bldrs URL structure as documented here is extended to support the "src" parameter:
http://host/share/v/src/${URL}
Example:
http://host/share/v/src/example.org%2Fblobs%2Fmyfile.ifc%3Fx%3D1%26y%3D2
This would load the file from https://example.org/blobs/myfile.ifc?x=1&y=2