Skip to content
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

minifying alters DataFactory types #172

Closed
ericprud opened this issue Apr 1, 2019 · 4 comments
Closed

minifying alters DataFactory types #172

ericprud opened this issue Apr 1, 2019 · 4 comments
Labels

Comments

@ericprud
Copy link

ericprud commented Apr 1, 2019

Problem

using a minifier changes the names of the classes that extend Term.

Reproduced

  1. Add N3.js to a vanilla module:
    MyModule.js
MyModule = (function () {
  let n3 = require('n3')
  return {
    some: "stuff",
    N3: n3
  }
})()

if (typeof require !== 'undefined' && typeof exports !== 'undefined')
  module.exports = MyModule
  1. package it with browserify or webpack:
    webpack.config.js
const path = require('path');
const TerserPlugin = require('terser-webpack-plugin');
const webpack = require("webpack");

module.exports = {
  entry: {
    "my-webapp-webpack": "./MyModule.js",
    "my-webapp-webpack.min": "./MyModule.js",
  },
  output: {
    filename: "[name].js",
    path: path.resolve(__dirname, 'browser'),
  },
  optimization: {
    minimize: true,
    minimizer: [new TerserPlugin({
      terserOptions: {
        "keep_classnames": false
      },
      include: /\.min\.js$/
    })]
  },
};
  1. Call it from some HTML:
    MyWebApp.html
    <script src="browser/my-webapp-webpack.min.js"></script>
    <scritp>
let f = MyModule.N3.DataFactory
const myQuad = f.quad(
  f.blankNode('s'),
  f.namedNode('http://a.example/p'),
  f.namepNode('http://a.example/o'),
  f.defaultGraph(),
);
console.log("quad:", JSON.stringify(myQuad))
console.log("subject termType:", myQuad.subject.termType)
    </script>
  1. check console; should see that the termTypes have been minified:
quad: {
  "subject": { "termType": "c", "value": "s" },
  "predicate": { "termType": "f", "value": "http://a.example/p" },
  "object": { "termType": "f", "value": "http://a.example/o" },
  "graph": { "termType": "p", "value": "" }
}
subject termType: c

Solution

There are two solutions: (1) advise users or (2) use static names for Term.typeName.

1 Tell minifier, e.g. terser (was uglifier) not to compress class names:

require('terser').minify(myModuleAsAString, {
  "keep_classnames": true
})

For webpack, that looks like:

  optimization: {
    minimize: true,
    minimizer: [new TerserPlugin({
      terserOptions: {
        "keep_classnames": true
      },
      include: /\.min\.js$/
    })]
  },
  1. Make the DataFactory Term.termType getters respond with static text:
--- packages/shex-cli/node_modules/n3/lib/N3DataFactory.js	2019-04-01 07:52:51.652880151 +0200
+++ /tmp/ediff4492TCm	2019-04-01 07:54:07.301991690 +0200
@@ -46,12 +46,19 @@
 
 
 // ## NamedNode constructor
-class NamedNode extends Term {}
+class NamedNode extends Term {
+  get termType() {
+    return 'NamedNode';
+  }
+}
 NamedNode.name = 'NamedNode';
 
 
 // ## Literal constructor
 class Literal extends Term {
+  get termType() {
+    return 'Literal';
+  }
   // ### The text value of this literal
   get value() {
     return this.id.substring(1, this.id.lastIndexOf('"'));
@@ -111,6 +118,10 @@
     super('_:' + name);
   }
 
+  get termType() {
+    return 'BlankNode';
+  }
+
   // ### The name of this blank node
   get value() {
     return this.id.substr(2);
@@ -123,6 +134,10 @@
     super('?' + name);
   }
 
+  get termType() {
+    return 'Variable';
+  }
+
   // ### The name of this variable
   get value() {
     return this.id.substr(1);
@@ -137,6 +152,10 @@
     return DEFAULTGRAPH || this;
   }
 
+  get termType() {
+    return 'DefaultGraph';
+  }
+
   // ### Returns whether this object represents the same term as the other
   equals(other) {
     // If both terms were created by this library,
@RubenVerborgh
Copy link
Member

NamedNode.name = 'NamedNode'; should fix that: #135
But I guess this broke with the ES6 rewrite…

Seems like we now need Object.defineProperty(NamedNode, 'name', { value: 'NamedNode' }). Bah.

Not sure which is the best performance-wise.

Maybe I should get rid of inheritance altogether.

@ericprud
Copy link
Author

ericprud commented Apr 2, 2019

I never figured out how to

x instanceof Term // or NamedNode or whatever

so I wouldn't miss inheritance as a user. There are places where I want to see if something is an RDFJS term but i do that with:

["NamedNode", "BlankNode", "Literal"].indexOf(x.termType) !== -1

Otherwise, maybe modern terser fixes #135?

@SimonShapiro
Copy link

I need at least a workaround for this. My use case is to modify ui elements based on the termType, so I need something predictable.

@SimonShapiro
Copy link

I'm pleased to report that method 1 above works fine. Thanks. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants