Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

dot-prop 4.0.0 is written in ES2015 #985

Closed
wants to merge 4 commits into from
Closed

Conversation

oligot
Copy link
Contributor

@oligot oligot commented Oct 11, 2016

Starting with version 4.0.0, the library dot-prop is now written in ES2015,
which means it doesn't work in IE anymore or when minifying the
code with uglifyjs as the code isn't transpiled.
This commit set es6 as format so that the code is transpiled and
works in IE and when minifying with uglifyjs

Starting with version 4.0.0, the library [dot-prop](https://github.com/sindresorhus/dot-prop)
is now [written in
ES2015](sindresorhus/dot-prop@88b6eb6),
which means it doesn't work in IE anymore or when minifying the
code with uglifyjs as the code isn't transpiled.
This commit set es6 as format so that the code is transpiled and
works in IE and when minifying with uglifyjs
@@ -0,0 +1,3 @@
{
"format": "es6"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"format": "esm" is preferred here. Also note to add jspmNodeConversion: false as well if that is intended.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see it isn't using modules, just ES features, so just the change to format: 'esm' should be fine here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
Out out curiosity, what is the difference between "format": "esm" and "format": "es6" ?
I always use the latter: should I always use the former instead ?

@guybedford
Copy link
Member

Sorry just looking at this again since it is CommonJS, how is this even working with this override? Are you sure you have tested this properly?

@oligot
Copy link
Contributor Author

oligot commented Oct 11, 2016

Yes, I've tested it.
Here is a small project with the override in the package.json file: https://github.com/oligot/jspm-override-dot-prop.
If you remove the override in the package.json file, reinstall the dependencies (rm -rf jspm_packages && jspm install) and try to build with npm run build, there is an error in uglifyjs (probably due to the fact that uglifyjs doesn't understand ES2015 yet)

Error: SyntaxError: Unexpected token name «i», expected punc «;» (line: 38, col: 11, pos: 5174)

     Error
         at new JS_Parse_Error (eval at <anonymous> (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/uglify-js/tools/node.js:28:1), <anonymous>:1545:18)
         at js_error (eval at <anonymous> (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/uglify-js/tools/node.js:28:1), <anonymous>:1553:11)
         at croak (eval at <anonymous> (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/uglify-js/tools/node.js:28:1), <anonymous>:2092:9)
         at token_error (eval at <anonymous> (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/uglify-js/tools/node.js:28:1), <anonymous>:2100:9)
         at expect_token (eval at <anonymous> (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/uglify-js/tools/node.js:28:1), <anonymous>:2113:9)
         at expect (eval at <anonymous> (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/uglify-js/tools/node.js:28:1), <anonymous>:2116:36)
         at regular_for (eval at <anonymous> (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/uglify-js/tools/node.js:28:1), <anonymous>:2357:9)
         at for_ (eval at <anonymous> (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/uglify-js/tools/node.js:28:1), <anonymous>:2353:16)
         at eval (eval at <anonymous> (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/uglify-js/tools/node.js:28:1), <anonymous>:2232:24)
         at eval (eval at <anonymous> (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/uglify-js/tools/node.js:28:1), <anonymous>:2139:24)
         at block_ (eval at <anonymous> (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/uglify-js/tools/node.js:28:1), <anonymous>:2432:20)
         at eval (eval at <anonymous> (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/uglify-js/tools/node.js:28:1), <anonymous>:2404:25)
         at function_ (eval at <anonymous> (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/uglify-js/tools/node.js:28:1), <anonymous>:2410:15)
         at eval (eval at <anonymous> (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/uglify-js/tools/node.js:28:1), <anonymous>:2235:24)
         at eval (eval at <anonymous> (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/uglify-js/tools/node.js:28:1), <anonymous>:2139:24)
         at block_ (eval at <anonymous> (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/uglify-js/tools/node.js:28:1), <anonymous>:2432:20)
         at minify (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/systemjs-builder/lib/output.js:79:11)
         at exports.writeOutputs (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/systemjs-builder/lib/output.js:153:14)
         at /home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/systemjs-builder/lib/builder.js:805:14
         at tryCatcher (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/systemjs-builder/node_modules/bluebird/js/release/util.js:16:23)
         at Promise._settlePromiseFromHandler (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/systemjs-builder/node_modules/bluebird/js/release/promise.js:510:31)
         at Promise._settlePromise (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/systemjs-builder/node_modules/bluebird/js/release/promise.js:567:18)
         at Promise._settlePromise0 (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/systemjs-builder/node_modules/bluebird/js/release/promise.js:612:10)
         at Promise._settlePromises (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/systemjs-builder/node_modules/bluebird/js/release/promise.js:691:18)
         at Promise._fulfill (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/systemjs-builder/node_modules/bluebird/js/release/promise.js:636:18)
         at Promise._resolveCallback (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/systemjs-builder/node_modules/bluebird/js/release/promise.js:431:57)
         at Promise._settlePromiseFromHandler (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/systemjs-builder/node_modules/bluebird/js/release/promise.js:522:17)
         at Promise._settlePromise (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/systemjs-builder/node_modules/bluebird/js/release/promise.js:567:18)
         at Promise._settlePromise0 (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/systemjs-builder/node_modules/bluebird/js/release/promise.js:612:10)
         at Promise._settlePromises (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/systemjs-builder/node_modules/bluebird/js/release/promise.js:691:18)
         at Promise._fulfill (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/systemjs-builder/node_modules/bluebird/js/release/promise.js:636:18)
         at Promise._resolveCallback (/home/oli/src/jspm-override-dot-prop/node_modules/jspm/node_modules/systemjs-builder/node_modules/bluebird/js/release/promise.js:431:57)

@guybedford
Copy link
Member

I get if it builds, but I mean does the code run? If you disable uglify does it work correctly?

@oligot
Copy link
Contributor Author

oligot commented Oct 20, 2016

Indeed, it doesn't work.
Here is the error I get in the console

Uncaught (in promise) Error: (SystemJS) require is not defined
    ReferenceError: require is not defined

So, it seems like with "format": "esm", we can only use ES6 module syntax (import/export).
Is there a way to configure a package so that it is recognized as an ES6 package (meaning, it will be transpiled with Babel) BUT is uses Node.js require instead of import to load dependencies ?

@guybedford
Copy link
Member

Exactly. This use case can be described by setting format: 'cjs' but then still configuring the Babel loader. But this would basically mean:

{
  "format": "cjs",
  "meta": {
    "*.js": {
      "loader": "babel"
    }
  }
}

The only problem with the above is it then makes the Babel loader a dependency of the project - there is no way in jspm currently to describe that the code uses the latest ES features without tying it to a transpiler instance. I think the edge cases of transpilation are still a little bit too inconsistent for that sort of conceptual model unfortunately.

@oligot
Copy link
Contributor Author

oligot commented Oct 21, 2016

I just tried it but it doesn't work (I'm still having this error when building): oligot/jspm-override-dot-prop@173dcd9.
Do you have an idea why it doesn't work ?

@guybedford
Copy link
Member

The above configuration is in jspm 0.17 only.

@oligot
Copy link
Contributor Author

oligot commented Oct 21, 2016

I've updated the project to jspm 0.17 but it still does not work.

system.src.js:122 Uncaught (in promise) Error: (SystemJS) require is not a function
    TypeError: require is not a function
        at execute (http://localhost:2000/jspm_packages/npm/dot-prop@4.0.0/index.js!transpiled:30:12)
    Error loading http://localhost:2000/app.js
        at execute (http://localhost:2000/jspm_packages/npm/dot-prop@4.0.0/index.js!transpiled:30:12)
    Error loading http://localhost:2000/app.js

@guybedford
Copy link
Member

Perhaps try setting format: 'cjs' as well explicitly in the override.

@oligot
Copy link
Contributor Author

oligot commented Oct 21, 2016

It's already set

@guybedford
Copy link
Member

@oligot it seems this is a bug. Try instead:

{
  "meta": {
    "*.js": {
      "loader": "plugin-babel",
      "format": "cjs"
    }
  }
}

@oligot
Copy link
Contributor Author

oligot commented Oct 27, 2016

It works now, thanks !
Note that I also had to put "modularRuntime": false in jspm.config.js

I'll update the PR accordingly.

@oligot
Copy link
Contributor Author

oligot commented Oct 27, 2016

Done

@guybedford
Copy link
Member

Glad to hear it is working. As mentioned previously though this isn't suitable for an override because it carries too many assumptions to provide to everyone. I would suggest keeping this as a local override in your project rather.

@oligot
Copy link
Contributor Author

oligot commented Oct 27, 2016

Ok, let's close the PR then.

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

Successfully merging this pull request may close these issues.

None yet

2 participants