-
Notifications
You must be signed in to change notification settings - Fork 571
Moving CJS to ES6 Modules #3379
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
Conversation
|
Ok going to change things a little as the DCE isn't working with how I have things currently: Normal export will be: var cube = function (x) {
return x * x * x;
}
export { cube };Foreign exports will need to go be a little more explicit: |
|
TypeScript has an option for choosing the output target ( |
|
I'm glad you started looking at ES6 imports and exports, but the real problem is DCE and bundling, as you've found out. There has been a lot of discussion about this in the past, and we decided that someone should go prototype a backend for ES6 first (so it's great that you have!). This can exist as a fork and be useful regardless of whether we can solve the DCE problem in the short term. Fixing DCE will involve changing |
|
Looks like the language-javascript has a PR (quintenkasteel/language-javascript#64) which adds partial ES6 support. I wonder if it's ok to use a branch of that project for this project's needs. |
|
@chexxor There's also the possibility that I proposed long ago of implementing just an import/export extractor for JS modules. That's all we care about, right? This would allow us to be much lazier about parsing. We don't care to actually parse the full module as long as we can accurately skip the parts we are uninterested in. |
|
Which turns out to be simpler and safer with es modules than cjs 🙂 |
|
I'm now pretty close but have a couple of niggly errors: // foreign.js
exports["insertCell'"] = function () {}
// index.js
import $foreign from "./foreign.js";
export const insertCell' = $foreign["insertCell'"];doesn't like and the other is a hanging I've tried various solutions with the way you can |
|
Should the ffi file use modules as well? Either way, I've seen // foreign.js
export const insertCell$prime = function () {};
// index.js
import { insertCell$prime } from "./foreign.js";
export const insertCell$prime = insertCell$prime; |
|
I'd be fine with just disallowing foreign imports of invalid JS identifiers when you're backed by a JS implementation. |
|
@spicydonuts that would be a good idea, been giving that a go. @michaelficarra just thinking of backwards compatibility really, because using prime is quite a common thing in PS / Haskell. I've managed to change the export but just searching for it's use like: |
|
I'm going to park this for the moment as not finding the time to finish it. Don't think I was too far away but will re attempt in the future if no one else has already done it 😄 |
|
Hi @Cmdv, Thanks for the work on this! I just tried out your branch (with a tiny patch to fix a bug, mentioned at the end) on the Halogen basic example. After running Tiny patch: --- a/src/Language/PureScript/CodeGen/JS.hs
+++ b/src/Language/PureScript/CodeGen/JS.hs
@@ -166,7 +166,7 @@ moduleToJs (Module _ coms mn _ imps exps foreigns decls) foreign_ =
-- a PureScript identifier. If the name is not valid in JavaScript (symbol based, reserved name) an
-- indexer is returned.
accessor :: Ident -> AST -> AST
- accessor (Ident prop) = accessorString $ mkString prop
+ accessor (Ident prop) = accessorString $ mkString $ properToJs prop
accessor (GenIdent _ _) = internalError "GenIdent in accessor"
accessor UnusedIdent = internalError "UnusedIdent in accessor"(Your code exports a function named |
|
@utkarshkukreti thanks I was literally thinking about starting this off again today. Think I went towards converting all keywords to using I can't recall if I did more work on this locally and broke things even further haha. I'll have a look and respond as this would be a cool feature 😄 Right so just been having a look, re you're last comment:
I've just checked and it does seem to output The other part that I have an issue with is before it used to do 2 different types of exports but now it only does one and I couldn't work out where this trailing just feels like with this working it should be able to be a lot more performant than this, do you feel it's more down to how the FFI's are not using ES6? I might try it out see if it does make a difference 😄 |
|
right so having experimented with converting an FFI to using ES6 export instead of CJS and it fails. This is probably done by the parser too so will need the ability to handle ES6 too, yet still being backwards compatible otherwise nothing will work unless all FFI code is converted to ES6!! that for sure doesn't sound fun. |
|
Yeah, looks like this is gonna require some more work. The language-javascript package recently got support for ES6 import/export statements. The compiler needs to be modified to support that. I was able to get webpack to work transparently with both ES6 and CommonJS modules: |
|
@utkarshkukreti looks like that merge was reverted in favor of implementing the features individually quintenkasteel/language-javascript#71 |
|
Ah, you're right. No support for import/export yet. 😞 |
|
that's cool though I might do it as the work has already been done just needs an individual PR 😄 |
|
@utkarshkukreti not sure if you or anyone else would be keen but now have 2 PR's on |
I've just been experimenting with generating ES6 modules, but before I went any further (add tests etc) wanted some opinions 😄
Think rather than a 1 to 1 conversion it would be great for users to opt in with say a flag
-es6?If this is a good idea could someone point me to an example of something using a flag.
Also if you look below at the output of the JS I ended up using:
instead of:
When I tested this
* asformat with Webpack 4 it just came up with lots of crazy warnings:The second issue was that foreign exports needed to be an object literal with key values and I wasn't able to use ES6 version of just:
export { a, b , c}because I'm not importing likeimport {a, b, c} from '../foreign.js'but I got around this by keeping the current functionality and adding
defaultkeyword to theexport.So far the output is as such:
Then foreign:
I tested this against Halogen Basic Demo with Webpack 4 with
> webpack --mode productionand the output file was still fairly large at
651 KiBso I'm not sure it's doing all of the DCE / Tree shacking properly.related to #1206 ¯\(ツ)/¯