add simple JSON pointer parsing to prevent data duplication

[patrickkettner]

fixes #813

idea is, given the following

file a.json

{
  "a": {
    "b": {
       "c": "hello!"
    }
  }
}

and file b.json

{
  "a": {
      "$ref": "a.b.c"
    }
}

then b.json would result in

{
  "a": "hello!"
}

[patrickkettner]

@ExE-Boss got an opinion?

[ExE-Boss]

Given that this is mainly used for mixins/interfaces, which are implemented by other things, and in many cases, the versions when the mixins were implemented tends to differ from when the features, that are now part of the mixins, were originally implemented on the objects that are now implementing the mixins.

Based on that, I believe that the way I described in #813 (comment) would allow us to support the cases where certain objects implemented the mixins at different times.

[patrickkettner]

@ExE-Boss sure. updated

[Elchi3]

This is a very cool idea. Thank you for this PR!
I will need to test this some more before merging. Also, I'm unsure if this slows down the data access in any way? There are now over 7000 features in over 1000 files in this repo already.
Tests and docs for this would be cool, too.

[ExE-Boss]

This happens during loading for EVERY FILE, so it ends up being O(n²), and it happens when the tree hasn’t been fully parsed (eg. we might have loaded api.*s and css.*, but not yet html.* or webextensions.*), which means that we might get weird stuff happening when a file in api.* imports something from webextensions.*.

[ExE-Boss]

Also, this violates the current schema, since $ref is a valid entry in an identifier type of type identifier, which can’t be a string:

browser-compat-data/schemas/compat-data.schema.json

Lines 112 to 121 in f724e4b

	"identifier": {
	"type": "object",
	"properties": {
	"__compat": { "$ref": "#/definitions/compat_statement" }
	},
	"patternProperties":{
	"^(?!__compat)[a-zA-Z_0-9-$@]*$" : { "$ref": "#/definitions/identifier" }
	},
	"additionalProperties": false
	},

Doing the following instead would be valid according to the current schema, since "<ref_to_import>" would be an empty object if it’s identical to the source:

"__import": {
  "<ref_to_import>": {
    // Override stuff here when necessary.
  }
}

I’m currently working on implementing my above thing.

[patrickkettner]

Also, I'm unsure if this slows down the data access in any way?

@Elchi3 the object parse takes a couple of ms on my laptop

it happens when the tree hasn’t been fully parsed (eg. we might have loaded api.s and css., but not yet html.* or webextensions.*),

@ExE-Boss how so? It happens after the result object is completely extended out

[ExE-Boss]

it happens when the tree hasn’t been fully parsed (eg. we might have loaded api.*s and css.*, but not yet html.* or webextensions.*),

how so? It happens after the result object is completely extended out

Except that the function load calls itself recursively:

browser-compat-data/index.js

Lines 40 to 66 in bc0e12c

	function load() {
	// Recursively load one or more directories passed as arguments.
	var dir, result = {};
	function processFilename(fn) {
	let fp = path.join(dir, fn);
	let extra;
	// If the given filename is a directory, recursively load it.
	if (fs.statSync(fp).isDirectory()) {
	extra = load(fp);
	} else if (path.extname(fp) === '.json') {
	extra = require(fp);
	}
	// The JSON data is independent of the actual file
	// hierarchy, so it is essential to extend "deeply".
	result = extend(true, result, extra);
	}
	for (dir of arguments) {
	dir = path.resolve(__dirname, dir);
	fs.readdirSync(dir).forEach(processFilename);
	}
	return processRefs(result, result);
	}

[patrickkettner]

Right, which means the ref parsing doesn’t occur until after the internal loops have been completed. This is the value that was being exported previously. If that wasn’t the case then the package would output incomplete data today

…

On Apr 17, 2018, at 11:57 AM, ExE Boss ***@***.***> wrote: it happens when the tree hasn’t been fully parsed (eg. we might have loaded api.*s and css.*, but not yet html.* or webextensions.*), how so? It happens after the result object is completely extended out Except that the function load calls itself recursively: https://github.com/mdn/browser-compat-data/blob/bc0e12c225f96156ebc5a5c7b3b056cc71f93c29/index.js#L40-L66 — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

[patrickkettner]

ping @Elchi3

[Elchi3]

Sorry, reviewing this and the other approach isn't in my priorities at the moment.
This PR at least needs a schema update and a test in the test/sample-data.json file so that we can run a basic test against the implementation. But then again, I'm currently focusing on getting the PR backlog down and finishing the data migration so we can retire old/static MDN compat tables. Thanks for your understanding.

[patrickkettner]

no worries, and thanks for the explanation.

…

On Thu, May 3, 2018 at 2:12 AM, Florian Scholz ***@***.***> wrote: Sorry, reviewing this and the other approach isn't in my priorities at the moment. This PR at least needs a schema update and a test in the test/sample-data.json file so that we can run a basic test against the implementation. But then again, I'm currently focusing on getting the PR backlog down and finishing the data migration so we can retire old/static MDN compat tables. Thanks for your understanding. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1681 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAcaBlMHMparCQtMyyrANI7KUjx1nZ33ks5tusoLgaJpZM4S5uvy> .

-- patrick

[Elchi3]

Bulk data updates through external sources and scripts are our preferred approach to better compat data right now and we're not currently considering de-duplicating version data. Therefore I'm closing this issue. Thanks for your thoughts here! Maybe we will reconsider this idea at a later stage.