Use vega-loader for validating links #47

ydlamba · 2018-03-10T08:47:50Z

Use vega-loader for validating links

It first checks whether the link is absolute or relative to the editor. If gets 404 then check whether it is relative to the caller and if still gets an error then throws it.

Fixes #30.

ydlamba · 2018-03-10T08:55:40Z

Currently, it is implemented for dataURL (like stocks.csv, etc). Should I implement the same for schema link?

domoritz

This approach won't work for nested specs. We need to create a custom loader and pass a baseUrl around. The loader would first try the baseUrl and if that returns a 404, try a link relative to the editor.

domoritz · 2018-03-10T22:47:55Z

src/components/app.tsx

+      if (error.target.status === 404) {
+        // Relative to caller
+        this.loaderPromise({baseURL: evt.origin}, dataURL).then(() => {
+          parsed['data']['url'] = evt.origin + dataURL;


This won't work if we have a nested spec. Instead of this approach, we should set a baseUrl in the state and create a custom Vega loader. Here is some inspiration: https://github.com/domoritz/draco/blob/master/draco-tools/src/shared/js/components/Visualization.js#L50

domoritz · 2018-03-10T22:48:16Z

src/components/app.tsx

+    const dataURL = parsed['data']['url'];
+
+    // Checking wheather link is absolute or relative to editor
+    this.loaderPromise({baseURL: 'https://vega.github.io/editor/'}, dataURL).then((data) => {


I think we should first check whether the URL is absolute. In that case, we won't do anything.

vega-loader already checks whether it is absolute or not. If not then I am checking it with relative to the editor which means we aren't doing anything when URL is absolute. I tested it.

Okay, this will might change when we switch to a custom loader.

domoritz · 2018-03-10T22:50:11Z

Currently, it is implemented for dataURL (like stocks.csv, etc). Should I implement the same for schema link?

This is irrelevant with the fix I am proposing above.

domoritz · 2018-04-03T06:19:31Z

I'd love to get this in. Can I help with anything @ydlamba?

ydlamba · 2018-04-03T11:15:31Z

I understand your approach, working on it.

domoritz · 2018-04-03T18:26:37Z

src/components/app.tsx

        console.info('[Vega-Editor] Received Message', evt.origin, data);
        // send acknowledgement
        const parsed = JSON.parse(data.spec);
+        let dataURL = parsed['data']['url'];


Let's delete this. We shouldn't expect a spec to have a URL data source.

domoritz · 2018-04-03T18:28:22Z

src/components/app.tsx

+    const editorURL = 'https://vega.github.io';
+    const loader = vega.loader();
+    const options = {
+      baseURL: this.props.baseURL


Instead of forcing the baseURL, try to check for every request whether it makes sense to change the baseURL. First, check whether there is data at the evt.origin that you grabbed above. If not, use the editor as the base.

domoritz

This isn't quite what I was hoping for. Can you override how the loader loads a single resource?

domoritz

Here is my proposal. Let me know what you think.

domoritz · 2018-04-05T03:39:25Z

src/components/app.tsx

+  /**
+   * Async function that updates the baseURL (if gets 404)
+   */
+  public async updateBaseUrl(url: string){


This can be deleted

domoritz · 2018-04-05T03:39:36Z

src/components/app.tsx

        console.info('[Vega-Editor] Received Message', evt.origin, data);
        // send acknowledgement
        const parsed = JSON.parse(data.spec);
+        if(parsed['data']['url']) {


delete this as well

domoritz · 2018-04-05T03:39:43Z

src/components/app.tsx

@@ -134,6 +159,12 @@ class App extends React.Component<Props & {match: any, location: any}> {
  }
 }

+function mapStateToProps(state: State, ownProps) {


delete this as well

domoritz · 2018-04-05T03:39:52Z

src/components/app.tsx

@@ -160,4 +194,4 @@ const mapDispatchToProps = function(dispatch) {
  };
 };

-export default withRouter(connect(null, mapDispatchToProps)(App));
+export default withRouter(connect(mapStateToProps, mapDispatchToProps)(App));


you can undo this now

domoritz · 2018-04-05T03:41:00Z

src/components/renderer/renderer.tsx

-      .logLevel(vega.Warn)
-      .initialize(Editor.chart);
-
+    Editor.view = new vega.View(runtime, {


Use something like this instead

const runtime = vega.parse(props.vegaSpec); const loader = vega.loader(); const originalHttp = loader.http; loader.http = async(url, options) => { try { return await originalHttp(url, {options, ...{baseURL: this.props.baseURL}}); } catch { return await originalHttp(url, options); } }; Editor.view = new vega.View(runtime, { loader: loader, logLevel: vega.Warn, }) .initialize(Editor.chart);

I haven't tested this but I think this communicates the idea.

I think there is some problem with loader.http, its baseURL option is not working. Instead loader.load is working fine.

I have created the issue as well.

ydlamba · 2018-04-05T22:14:37Z

PR is now updated. I have tested this and working as expected (just with few minor changes). You can also have a look.

domoritz · 2018-04-06T08:48:04Z

src/components/renderer/renderer.tsx

+    // Custom Loader
+    loader.load = async(url, options) => {
+      try {
+        return await originalLoad(url, {options, ...{baseURL: this.props.baseURL}});


it should be ...options, right?

what if options is undefined?

shouldn't custom options override our baseurl? (this one os less important and you can ignore it)

I guess you are thinking about options in options. Actually, we need to add baseURL directly in the options.

...options will append baseURL in options of options (nested object) while options will directly add baseURL in options. I tested it, working fine.

We don't need to worry about options (not options in options) loader is taking care of it.

Ignored for now.

So, if we go for loader.load(url, {options: {baseURL: this.props.baseURL}}) it won't work.

I'm confused. I thought the behavior of ...options and just options is flipped from how you describe it. I will test it when I'm.at a computer again.

I think I was right.

options = {foo:42, bar: 123} $ {...options, ...{baseUrl: 'foo.bar'}} {foo: 42, bar: 123, baseUrl: "foo.bar"} // good $ {options, ...{baseUrl: 'foo.bar'}} {options: {foo: 42, bar: 123}, baseUrl: "foo.bar"} // not good

See https://github.com/vega/vega-loader#loader for documentation of the loader. We want all options at the top level.

Yeah. This is correct. I was thinking it the wrong way, nevermind. I've updated the PR.

Last question. Why override load and not http?

Because loader.http method performs an HTTP request with a post-sanitization URL i.e we don't have baseURL option with this method. On the other hand, loader.load does all processing itself.
For more info check this vega/vega-loader#7.

domoritz · 2018-04-06T08:49:23Z

Sweet, just one comment.

ydlamba · 2018-04-06T19:38:09Z

Changes are done, please let me know if I am missing something.

ydlamba changed the title ~~Update~~ Use vega-loader for validating links Mar 10, 2018

ydlamba requested a review from domoritz March 10, 2018 08:55

vega deleted a comment from ydlamba Mar 10, 2018

domoritz requested changes Mar 10, 2018

View reviewed changes

ydlamba changed the title ~~Use vega-loader for validating links~~ [WIP] Use vega-loader for validating links Mar 13, 2018

ydlamba force-pushed the ydl/vega-loader branch 3 times, most recently from f3a60a8 to 10d17c6 Compare April 3, 2018 12:27

domoritz reviewed Apr 3, 2018

View reviewed changes

domoritz requested changes Apr 3, 2018

View reviewed changes

ydlamba force-pushed the ydl/vega-loader branch from 10d17c6 to 29eec89 Compare April 4, 2018 15:52

domoritz requested changes Apr 5, 2018

View reviewed changes

ydlamba added 3 commits April 6, 2018 02:45

Add baseURL to state

25d386e

Set baseURL as caller's URL

00a9e10

Create custom loader

b1d52bb

ydlamba force-pushed the ydl/vega-loader branch from 29eec89 to b1d52bb Compare April 5, 2018 22:11

domoritz reviewed Apr 6, 2018

View reviewed changes

ydlamba force-pushed the ydl/vega-loader branch from 8b90f17 to b1d52bb Compare April 6, 2018 19:34

Minor update in custom loader

48339fa

domoritz merged commit dbaa0e5 into master Apr 12, 2018

ydlamba changed the title ~~[WIP] Use vega-loader for validating links~~ Use vega-loader for validating links Apr 12, 2018

ydlamba deleted the ydl/vega-loader branch April 12, 2018 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use vega-loader for validating links #47

Use vega-loader for validating links #47

ydlamba commented Mar 10, 2018 •

edited

ydlamba commented Mar 10, 2018

domoritz left a comment

domoritz Mar 10, 2018

domoritz Mar 10, 2018

ydlamba Mar 10, 2018 •

edited

domoritz Mar 10, 2018

domoritz commented Mar 10, 2018

domoritz commented Apr 3, 2018

ydlamba commented Apr 3, 2018

domoritz Apr 3, 2018

domoritz Apr 3, 2018

domoritz left a comment

domoritz left a comment

domoritz Apr 5, 2018

domoritz Apr 5, 2018

domoritz Apr 5, 2018

domoritz Apr 5, 2018

domoritz Apr 5, 2018

ydlamba Apr 5, 2018

ydlamba Apr 5, 2018

ydlamba commented Apr 5, 2018

domoritz Apr 6, 2018 •

edited

ydlamba Apr 6, 2018 •

edited

ydlamba Apr 6, 2018

domoritz Apr 8, 2018

domoritz Apr 12, 2018

ydlamba Apr 12, 2018

domoritz Apr 12, 2018

ydlamba Apr 12, 2018

domoritz commented Apr 6, 2018

ydlamba commented Apr 6, 2018 •

edited

Use vega-loader for validating links #47

Use vega-loader for validating links #47

Conversation

ydlamba commented Mar 10, 2018 • edited

ydlamba commented Mar 10, 2018

domoritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ydlamba Mar 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domoritz commented Mar 10, 2018

domoritz commented Apr 3, 2018

ydlamba commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domoritz left a comment

Choose a reason for hiding this comment

domoritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ydlamba commented Apr 5, 2018

domoritz Apr 6, 2018 • edited

Choose a reason for hiding this comment

ydlamba Apr 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domoritz commented Apr 6, 2018

ydlamba commented Apr 6, 2018 • edited

ydlamba commented Mar 10, 2018 •

edited

ydlamba Mar 10, 2018 •

edited

domoritz Apr 6, 2018 •

edited

ydlamba Apr 6, 2018 •

edited

ydlamba commented Apr 6, 2018 •

edited