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

Import single value (for security reasons) #2310

Open
doomedramen opened this issue Nov 21, 2018 · 33 comments
Open

Import single value (for security reasons) #2310

doomedramen opened this issue Nov 21, 2018 · 33 comments

Comments

@doomedramen
Copy link

My project (a node web app) has a file (config.js) sitting at its root. It contains passwords, preferences etc. I would like to pull one of these into my front end js file that is being built with parcel but when it is built the entire config.js file copied to the output.

For example:

import {apdexT} from '../../../config.js';

in my code I expect to see the val apdexT but I see the entire contents of ../../../config.js
Is there a way for the rest of the fie (that is not used in the file built with parcel) to be ignored?

Thank you in advance.

@doomedramen
Copy link
Author

in my built code there a like that reads:

module.exports={port:3001,db:"uptime",secret:"Change Me",showSitesToUnauth:!0,graphsOnIndex:!0,allowRegistration:!1,apdexT:.2,monitoring:{localRunner:!0,interval:5},slack:{enabled:!0,webhookURL:"..."},airdale:{enabled:!1,apiKey:""}};

This is the entire contents of the config.js file, only apdexT should have been imported.

@mischnic
Copy link
Member

You could use an envfile

@doomedramen
Copy link
Author

@mischnic do you have any suggestions?

@mischnic
Copy link
Member

You could create a .envfile containing

API_KEY=0123456

and then use it like this: process.env.API_KEY.

(Inspired by #2299)

@macklinu
Copy link

macklinu commented Dec 12, 2018

I ran into a similar problem trying to import just the version from a package.json file:

package.json

{
  "name": "example",
  "description": "A description",
  "version": "1.0.0",
  "scripts": {
    "build": "parcel build index.html"
  },
  "devDependencies": {
    "parcel-bundler": "1.10.3"
  }
}

index.html

<html>
  <body>
    <script src="./index.js"></script>
  </body>
</html>

index.js

import { version } from "./package.json";

console.log(version);

When running yarn build (or npm run build) in the above example, the output dist contains a JavaScript bundle that looks like:

dist/parcel-json.ff909ce6.js

parcelRequire=function(e,r,n,t){var i="function"==typeof parcelRequire&&parcelRequire,o="function"==typeof require&&require;function u(n,t){if(!r[n]){if(!e[n]){var f="function"==typeof parcelRequire&&parcelRequire;if(!t&&f)return f(n,!0);if(i)return i(n,!0);if(o&&"string"==typeof n)return o(n);var c=new Error("Cannot find module '"+n+"'");throw c.code="MODULE_NOT_FOUND",c}p.resolve=function(r){return e[n][1][r]||r},p.cache={};var l=r[n]=new u.Module(n);e[n][0].call(l.exports,p,l,l.exports,this)}return r[n].exports;function p(e){return u(p.resolve(e))}}u.isParcelRequire=!0,u.Module=function(e){this.id=e,this.bundle=u,this.exports={}},u.modules=e,u.cache=r,u.parent=i,u.register=function(r,n){e[r]=[function(e,r){r.exports=n},{}]};for(var f=0;f<n.length;f++)u(n[f]);if(n.length){var c=u(n[n.length-1]);"object"==typeof exports&&"undefined"!=typeof module?module.exports=c:"function"==typeof define&&define.amd?define(function(){return c}):t&&(this[t]=c)}return u}({"uc/H":[function(require,module,exports) {
module.exports={name:"example",description:"A description",version:"1.0.0",scripts:{build:"parcel build index.html"},devDependencies:{"parcel-bundler":"1.10.3"}};
},{}],"Focm":[function(require,module,exports) {
"use strict";var e=require("./package.json");console.log(e.version);
},{"./package.json":"uc/H"}]},{},["Focm"], null)
//# sourceMappingURL=/parcel-json.ff909ce6.map

However, this would be the desired output bundle, since I'm only importing version.

diff --git a/dist/parcel-json.ff909ce6.js b/dist/parcel-json.ff909ce6.js
index d00e1dc..f77ef53 100644
--- a/dist/parcel-json.ff909ce6.js
+++ b/dist/parcel-json.ff909ce6.js
@@ -1,5 +1,5 @@
 parcelRequire=function(e,r,n,t){var i="function"==typeof parcelRequire&&parcelRequire,o="function"==typeof require&&require;function u(n,t){if(!r[n]){if(!e[n]){var f="function"==typeof parcelRequire&&parcelRequire;if(!t&&f)return f(n,!0);if(i)return i(n,!0);if(o&&"string"==typeof n)return o(n);var c=new Error("Cannot find module '"+n+"'");throw c.code="MODULE_NOT_FOUND",c}p.resolve=function(r){return e[n][1][r]||r},p.cache={};var l=r[n]=new u.Module(n);e[n][0].call(l.exports,p,l,l.exports,this)}return r[n].exports;function p(e){return u(p.resolve(e))}}u.isParcelRequire=!0,u.Module=function(e){this.id=e,this.bundle=u,this.exports={}},u.modules=e,u.cache=r,u.parent=i,u.register=function(r,n){e[r]=[function(e,r){r.exports=n},{}]};for(var f=0;f<n.length;f++)u(n[f]);if(n.length){var c=u(n[n.length-1]);"object"==typeof exports&&"undefined"!=typeof module?module.exports=c:"function"==typeof define&&define.amd?define(function(){return c}):t&&(this[t]=c)}return u}({"uc/H":[function(require,module,exports) {
-module.exports={name:"example",description:"A description",version:"1.0.0",scripts:{build:"parcel build index.html"},devDependencies:{"parcel-bundler":"1.10.3"}};
+module.exports={version:"1.0.0"};
 },{}],"Focm":[function(require,module,exports) {
 "use strict";var e=require("./package.json");console.log(e.version);
 },{"./package.json":"uc/H"}]},{},["Focm"], null)

Would this be a welcome addition to Parcel, and if so, could someone please help point me (or another person who would like to contribute) in the direction of where to add/edit code for this feature in the codebase? 😃

@mischnic
Copy link
Member

Not sure how easy this is to do. You basically want to do tree shaking for json assets.

@macklinu
Copy link

My use case feels similar to this issue (tree shaking for JS assets) but not quite since my example is importing a JSON asset. Should I track JSON asset tree-shaking in a separate ticket (don't see anything open for this at the moment)?

@DeMoorJasper
Copy link
Member

@macklinu JSON files get transformed to JS and JS gets transformed by babel and gathers information for treeshaking so in theory it should already do this.

Feel free to experiment with it using parcel build index.html --experimental-scope-hoisting

@mischnic
Copy link
Member

mischnic commented Dec 12, 2018

so in theory it should already do this.

In practice: (with treeshaking)

(function() {
	var a = {};
	a = {
		version: "1.2.3",
		scripts: {
			build: "parcel build --target node --experimental-scope-hoisting index.js",
			"build-ts": "parcel build --target node index.js"
		},
		devDependencies: {
			"@babel/core": "^7.2.0",
			"@babel/preset-env": "^7.2.0"
		}
	};
	console.log(a.version);
})();

@mischnic
Copy link
Member

mischnic commented Dec 12, 2018

If I recall correctly, parcel's scope hoisting/shaking.js visitor only removes all unreferenced variables and doesn't go into the "object" level.

Terser doesn't do it either, try it here: https://xem.github.io/terser-online/.

@mischnic
Copy link
Member

mischnic commented Dec 12, 2018

Oh: terser can't optimize this

(function() {
	var a = {};
	a = {
		version: "1.2.3",
		// ...
	};
	console.log(a.version);
})();

// result:
// no difference

But only this:

(function() {
	var a = { // <---- no reassignment
		version: "1.2.3",
		// ...
	};
	console.log(a.version);
})();

// result:
console.log("1.2.3")

Parcel's scope hoisting seems to always do this:

var $ucH$exports = {};
$ucH$exports = {
	// ...
}

@DeMoorJasper
Copy link
Member

@mischnic that's probably because it can cause side-effects pretty easily

@mischnic
Copy link
Member

mischnic commented Dec 13, 2018

that's probably because it can cause side-effects pretty easily

But there aren't any with a JSON import, right?


Ideally, there would be no extra var $...$exports = {}; line if the asset's first access to module.exports was reassigning it (and not adding a property). (This should not have side-effects)

// ---- source
module.exports = {
  // json data
};

// ---- output
var $ucH$exports = {};
$ucH$exports = {
  // json data
};

@DeMoorJasper
Copy link
Member

@mischnic not sure why this is done, thought it was side-effects, probably is. But probably on more complex scenarios than this simple example.

cc @fathyb would be great to get your input on this.

@mischnic
Copy link
Member

not sure why this is done, thought it was side-effects, probably is. But probably on more complex scenarios than this simple example.

This wouldn't work with getters.
But I just realised that terser doesn't run on the packager level, so this would be required after fixing the hoisting:

--- a/packages/core/parcel-bundler/src/packagers/JSConcatPackager.js
+++ b/packages/core/parcel-bundler/src/packagers/JSConcatPackager.js
@@ -542,6 +542,11 @@ class JSConcatPackager extends Packager {
       }
     }

+    output = require("terser").minify(output, { // options from transforms/terser.js
+      warnings: true,
+      safari10: true,
+    }).code;
+
     await super.write(output);
   }

@fathyb
Copy link
Contributor

fathyb commented Dec 13, 2018

We do var exports = {}; exports = actual_exports; to be compatible with module.exports reassignements.
I previously made a PR (#1939) to "optimize" this but it added too much complexity and Terser already does it so I closed it. Please see @kzc comment's: #1939 (comment) to fix this:

The uglify/terser unsafe compress option is actually safe for sane code - anything that does not modify builtins like polyfills. It's just being conservative by default.

Application code should specify --toplevel (or --module) to get rid of unused top level variable declarations.

You can configure Terser by adding a .terserrc file to your project.

@mischnic
Copy link
Member

mischnic commented Dec 13, 2018

Well, the {compress: {unsafe: true}} options doesn't change to output. {module: true} and {toplevel: true} both result in (parameter should be "1.2.3"):

(function () {console.log({});})();

@mischnic
Copy link
Member

mischnic commented Dec 13, 2018

Terser already does it so I closed it

How can terser do this already? As far as I can tell, only the assets are optimized by terser. This case can only be done by the packager

@kzc
Copy link

kzc commented Dec 13, 2018

Oh: terser can't optimize this

$ cat test.js
(function() {
	var a = {};
	a = {
		version: "1.2.3",
		scripts: {
			build: "parcel build --target node --experimental-scope-hoisting index.js",
			"build-ts": "parcel build --target node index.js"
		},
		devDependencies: {
			"@babel/core": "^7.2.0",
			"@babel/preset-env": "^7.2.0"
		}
	};
	console.log(a.version, a.devDependencies["@babel/core"]);
})();

Enabling unsafe is enough for this particular example, but I generally optimize with these options:

$ cat test.js | terser -c unsafe,passes=3,pure_getters --toplevel
console.log("1.2.3","^7.2.0");

btw, Parcel uses Terser in a suboptimal way in order to improve build times. For smaller bundle size Terser should only be used once per bundle - not per JS asset with only a final mangle at the end.

You can configure Terser by adding a .terserrc file to your project.

Is this .terserrc intended for each JS asset or the final bundle? They have different goals and should probably have different configs. The issue was never resolved AFAIK.

@mischnic
Copy link
Member

mischnic commented Jan 9, 2019

btw, Parcel uses Terser in a suboptimal way in order to improve build times. For smaller bundle size Terser should only be used once per bundle - not per JS asset with only a final mangle at the end.

@fathyb @devongovett This has come up several times now. Are there any reasons not to do it this way?

@devongovett
Copy link
Member

Last I checked it was much faster to run terser at the asset level and only mangle the top-level scope at the end rather than run terser over the final output, which could be huge. There also wasn't a very big difference in output size that I saw, but perhaps you've run into something. Did you try running terser over the final bundle to see if it got smaller?

@mischnic
Copy link
Member

Did you try running terser over the final bundle to see if it got smaller?

Mh, the only case where it helps seems to be this particular issue ("inlining a object.key lookup"). (In one project, the bundle was actually larger when removing the asset-level terser).

@kzc
Copy link

kzc commented Jan 13, 2019

See: #1685 (comment)

Given:

$ parcel -V
1.11.0
$ cat app.js
import {Sum} from './lib.js'
(new Sum(10, 4)).run();
$ cat lib.js
export class Sum {
    constructor(x, y) {
        this.x = x;
        this.y = y;
    }
    calc() {
        return this.x + this.y;
    }
    run() {
        console.log("Sum:", this.x, this.y, "=>", this.calc());
    }
}
export class Multiply {
    constructor(x, y) {
        this.x = x;
        this.y = y;
    }
    calc() {
        return this.x * this.y;
    }
    run() {
        console.log("Multiply:", this.x, this.y, "=>", this.calc());
    }
}
export class Divide {
    constructor(x, y) {
        this.x = x;
        this.y = y;
    }
    calc() {
        return this.x / this.y;
    }
    run() {
        console.log("Divide:", this.x, this.y, "=>", this.calc());
    }
}
export class Minus {
    constructor(x, y) {
        this.x = x;
        this.y = y;
    }
    calc() {
        return this.x - this.y;
    }
    run() {
        console.log("Minus:", this.x, this.y, "=>", this.calc());
    }
}

Per-asset minification enabled yields a 1216 byte bundle:

$ parcel build ./app.js --experimental-scope-hoisting
✨  Built in 448ms.

dist/app.js    1.19 KB    353ms

Per-asset minification disabled - this unminified bundle size is not important:

$ parcel build ./app.js --no-minify --experimental-scope-hoisting
✨  Built in 362ms.

dist/app.js    2.51 KB    268ms

Running that unminified bundle through terser mangle/compress yields a 564 byte bundle:

$ terser dist/app.js -mc | wc -c
     564

@mischnic
Copy link
Member

Some notes:

  • This is mostly because of the babel class transpilation, when ES6 is targeted, the difference is less significant.
  • It's very important to keep comments during babel code generations (for the PURE annotations), I think this is what skewed my test from earlier today:
    comments: !packager.options.minify

@mischnic
Copy link
Member

I've created a POC for testing: yarn add @mischnic/parcel-bundler.
Changes: https://github.com/parcel-bundler/parcel/compare/experiment-scopehoist-terser

@mischnic
Copy link
Member

mischnic commented Jan 28, 2019

Rollup (REPL) handles this just fine (and the POC above as well), but I don't think they're using terser?

import { cube } from './maths.js';

if(cube)
	console.log("YES");
else
	console.log("NO");


// maths.js
export const cube = false;
export const square = false;

@doomedramen
Copy link
Author

Thank you for the heads up about how Rollup handles this @mischnic

@mischnic
Copy link
Member

mischnic commented Feb 27, 2019

@kzc What made the non-global terser invocation perform bad in this situations has been that Parcel didn't know about the __PURE__ comments, with the babel classes (your example) and when added manually (#2646). (Though it wouldn't solve this specific issue)
(I'm still not saying that the per-asset parser is the best way regarding code size).

As an alternative to running terser on the whole bundle, I made the scope hoisting algorithm be aware of these PURE comments (at its core a 3 line change).
The only issue I've run into is that terser doesn't keep the PURE comments when we run it on the asset, cli equivalent:

$ echo "var a = /*@__PURE__*/foobar();" | terser -c --comments "/__PURE__/"
var a=/* */foobar();
$ echo "var a = /*@__PURE__*/foobar();" | terser -c --comments "all"
var a=/* */foobar();

Can this be achieved with terser, is this a bug or I'm afraid it's intended?

WIP branch: https://github.com/parcel-bundler/parcel/compare/treeshake-pure

@kzc
Copy link

kzc commented Feb 27, 2019

I no longer work on terser. The pure annotation removal was intentionally introduced in uglify-js (and inherited by terser) to prevent the comments incorrectly shifting to non pure code after the compress pass and producing incorrect results. It was the simplest solution at the time to avoid some rather tricky corner cases.

Rollup has also recently implemented uglify-js compatible pure annotations and incorporated the corresponding uglify-js tests. This PR discusses the issues involved in its implementation: rollup/rollup#2429

@mischnic
Copy link
Member

mischnic commented Feb 27, 2019

Thank you for your quick response

It was the simplest solution at the time to avoid some rather tricky corner cases.

Makes sense.

Rollup has also recently implemented uglify-js compatible pure annotations and incorporated the corresponding uglify-js tests.

My first idea was: 1) run terser on asset level 2) tree shake more intelligently using the pure comments to prevent running terser on the whole output.
The first part is impossible (because the pure comments get removed) and the second requires quite a bit of effort (I'm not sure if this more difficult to do with parcel's current logic compared to Rollup). and while the pure comment handling seems doable, it wouldn't add much value because we can't run terser beforehand (asset) and don't want to run terser afterwards (bundle).
If we want to run terser and have comparable bundle sizes, we'll have to run terser on the output...
@devongovett
Another perspective: the pure handling could help tree shake more code, so that terser has to process less code on the bundle level

@kzc
Copy link

kzc commented Mar 12, 2021

@mischnic I don't know where this issue stands and perhaps you guys found another solution with terser in the past couple of years, but I thought I'd pass on that uglify-js just implemented the ability to emit pure annotations in minified output: mishoo/UglifyJS#4763. This annotations feature has been committed but not yet released. uglify-js now supports ES6+ up to ES2020. It's a different code base from terser and each have some optimizations that the other may lack.

@mischnic
Copy link
Member

Thanks. Parcel 2 already runs Terser at the end (so on the bundled output).

Should be pretty easy to write an optimizer plugin that uses uglify-js instead, might be interesting to compare the two.

@kzc
Copy link

kzc commented Mar 13, 2021

@mischnic Cool. Please share your findings and open an issue with uglify-js if there's a problem.

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

No branches or pull requests

7 participants