import into ES6 project unclear

[johanneswilm]

Hey,
this edtf module is somehow differently exported than all other packages on npmjs, and I don't quite get why. it says it's written in ES6, but from what I can tell, it's really using commonjs module imports. I expected to be able to do imports like this in ES6:

import {parse} from 'edtf'

That is working will all other packages, except this one. Is there some other ways to import it in ES6?

[inukshuk]

Node.js still uses require / CommonJS modules by default, with stable ES6 module support planned for Node.js 10 (I think?) but available behind a flag since 8.5 (using the .mjs extension) so I doubt that this module is exported differently than all the other modules on NPM : )

When you say you're trying to 'import it in ES6' -- what platform or module loader are you using? In Node.js you can load existing CommonJS modules using import, but named imports will not work because the module must be evaluated once (and ES6 module import is static). Therefore, import edtf from 'edtf' should work (but import {parse} from 'edtf' will not). For example, I just tried this:

$ cat test.mjs 
import edtf from 'edtf'
console.log(edtf.parse('2006'))
$ node -v
v8.9.4
$ node --experimental-modules test.mjs 
(node:14242) ExperimentalWarning: The ESM module loader is experimental.
{ type: 'Date', level: 0, values: [ 2006 ] }

[johanneswilm]

I doubt that this module is exported differently than all the other modules on NPM : )

Actually it seems to be packaged differently than everything else. Because all other modules, as you say, are also in CommonJS format, yet it is no problem to import them with the normal ES6 syntax. I have no problem importing from the modules I created myself on npmjs (source in ES6 and transpiled to commonjs), nor importing any other package, no matter whether it was originally written with CommonJS or ES6 modules as it's all transpiled to CommonJS.

EDTF is the one big exception. Using the syntax you mention import edtf from 'edtf' gives this error:

Error: Could not resolve './en-US' from node_modules/edtf/locale-data/index.js while parsing file

[inukshuk]

It is different than many modules in that it uses ES6 which are supported by Node.js with no fallback, transpiled code etc. for platforms without native support for those features. But it is a normal CommonJS module and as you can see from the example I posted above the experimental ES6 module loader in Node.js 8.9 can load it.

Anyway, your module loader (you still haven't told me which module loader you're using) does not seem to be able to resolve the locale data, which is in a .json file; I suppose that's the reason why it fails. We can turn the file into a normal .js file to work around this.

[johanneswilm]

It is different than many module

I also write mainly ES6 nowadays, and the project I am trying to use your edtf in is ES6 itself. Yet because this package doesn't follow the convention of writing the original code in ES6 (including imports) and then transpiling, I cannot use the package as it is but have to do some transpiling myself first.

I am using browserify and I spent about a day getting it to work last time. Now I was trying out rollup and again it seems I have to spend another day to get it to work with EDTF.

You can see how I got it to work with browserify here:

https://github.com/fiduswriter/biblatex-csl-converter/blob/master/package.json#L33

and here:

https://github.com/fiduswriter/biblatex-csl-converter/blob/master/src/edtf.js#L1

Unfortunately this does not work with rollup or any other solution.

What is the argument against just following the convention of having a src folder with the ES6 sources inside, complete in ES6 format, modules and all, but for the purpose of npmjs to offer a dist-folder with a transpiled version?

[inukshuk]

When using ES6 modules in your project you can treat this one like any other CommonJS module: that is, you can import it just fine, you just can't use named imports. The load error you were reporting has to do with requiring a JSON file without stating the extension explicitly (see here). I just added the extension to the require on master, can you tell me if that fixes the problem for you? If it does I'll push a it to NPM shortly.

The code is written using CommonJS because that's the only way we can load the files directly into Node.js and Electron, including the debugger, which is where we're using the library. I don't have anything against bundling a version with RollUp, but it's simply not something we've needed to do.

[johanneswilm]

Last time I researched this, I don't think it accepted any ES6 code in the dist node_modules folder at all. It simply required everything to be transpiled, which is why everyone does that. But things may have changed since then. So you are that now the following should work, right?

import edtf from 'edtf'
edtf.parse(...)

[inukshuk]

Using Node.js module loader import edtf from 'edtf' will work. This should work with RollUp too, because you got the json error above -- to work around that I just made the requires explicit on master. If you install the module from master and try again with your previous setup (i.e., the one that produced the error about unresolvable 'en-US') I think it will work (judging by the thread I linked to above).

[johanneswilm]

I made the changes you committed to my local edtf copy. That now creates a "Error: Unexpected token while parsing file" but it then does not tell me which file this is in. Same behavior for browserify/babelify and rollup. I tried looking at verbose output as well.

[inukshuk]

OK, thanks for looking into it. I'll give rollup a try later and report back.

[inukshuk]

Having explored RollUp a bit, the 'unexpected token' error is almost definitely because you're missing the 'rollup-plugin-json' in your config. Using that I hit another error which is caused by an array destructuring assignment in my code, which, I don't know, why it would not be supported (you probably would not hit the error if you do any sort of ES6 transpiling which I'm trying to avoid). These are just two assignments, so I changed them in the code and RollUp creates bundles just fine.

Now, this should be enough for you to bundle the module with RollUp just fine: you'll need the json, node-resolve, and commonjs plugins. The only built-in used by the module is 'assert' so for a build which can run outside of Node.js you'd also need to enable the (node-globals and node-builtins) plugins.

Alternatively, we could include bundles for distribution, but there are quite a number of different builds people might want: should the bundle include assert or not? Use CommonJS or UMD or both? (That's exactly the sort of decision making I wanted to avoid).

[johanneswilm]

Thank you so much for investigating.

you probably would not hit the error if you do any sort of ES6 transpiling which I'm trying to avoid

For us, and likely just about all projects facing browsers, that's not really practical. We need to support today's browsers so there is no way to get around some transpiling at least. We do limit users to one of the last two browser versions, and that is already considered an extreme measure.

As for bundling -- I don't know for sure, but it seems that everyone else is following a convention on this -- I would guess CommonJS as that is what seems to still be the default. So far it has never been problem for me working with any package I found on npmjs, with the exception of edtf.js

My guess is that we'll all eventually drop transpiling - probably once Node supports ES6 modules by default. Probably if you had started this package say 1 to 2 years from now, you could avoid transpiling entirely.

[inukshuk]

Many ES6 features cannot be transpiled; most are ported to more or less equivalent ES5 code, but that's not the same thing. So I hope you will understand that we want to use native generators, native proxies, native weak maps, or native classes with super calls etc. and not an ES5 approximation which may or may not be fully compliant to the ES6 spec. (More so since we're targeting Node.js and Electron which have been supporting all these features for years; and want to be able to load the original code into a debugger.)

That said, we can include bundles and even transpiled bundles in the module: yes. Although this adds considerable maintenance overhead: the dependencies for packaging and transpiling alone outnumber those of the module itself; not to speak of the time spent setting this up and maintaining it going forward, when we'll never use the bundled code ourselves. But OK, I'm willing to do this and include bundles, but I have not yet found the single convention you mention that everyone agrees on.

If we provide transpiled bundles, which ES6 features exactly should we transpile? All of them? Some users of the library will target different browsers than others and might want to transpile different things. I imagine they would be annoyed if they had to include a generator polyfill for example, even though their target platform supports generators.

Let's take your case, for example: you know exactly which browsers you want to target so I'd argue you would want to do transpile the code yourself and pick the appropriate babel preset for instance. If you use RollUp and add the plugins above, plus a babel plugin or similar -- isn't that exactly what you need? Since you are using ES6 yourself you're probably doing this anyway, right?

Anyway, so if we provide bundles, what is the best way to go about it? Include one CommonJS bundle.js for Node.js users (not including built-ins), an UMD (?) bundle.umd.js including Node.js built-ins for modern browsers; and UMD bundle.umd.compat.js, including Node.js built-ins and transpiled to ES5 for old browsers?

[johanneswilm]

Have you looked at the dependencies you are using (nearley and randexp) as well their references and seen how they do their bundling? Nearley probably doesn'tbundle anything, but its dependencies might. Whatever they are targetting should probably work. I agree that it doesn't have to be more than that. As long as the current versions of the transpilers themselves can work with an edtf dependency (webpack, rollup, browserify/, etc.) anyone who needs a built to target IE6 can easily create that himself.

[johanneswilm]

One question as to how you use assert: So far, I have only run into assert for testing and debugging. But looking at your code it seems you are using it in the general code. Am I reading that correctly? So it's not an option to strip all asserts using rollup-plugin-strip, correct?

[johanneswilm]

Have you seen this from the rollup instructions?:

"Publishing ES6 Modules
To make sure your ES6 modules are immediately usable by tools that work with CommonJS such as Node.js and webpack, you can use Rollup to compile to UMD or CommonJS format, and then point to that compiled version with the main property in your package.json file. If your package.json file also has a module field, ES6-aware tools like Rollup and webpack 2 will import the ES6 module version directly."

[johanneswilm]

Hey, @inukshuk. I was now able to use rollup to make a bundle. But the bundle doesn't work in older browsers. Basically, rollup and other ES6+ tools seem to expect a fully ES6-based source (including import/export statements) through an entry point specified as "module" in the package.json, and a fully ES5-based bundle as "main". What confuses them is mixing the two.

I wonder: I assume you are using EDTF as a dependency for something, not just as a program for itself. Could you then not provide a full ES6 and a full ES5 version under "module" and "main" of this library and then in the main project which imports these, you add a bundler that turns it into the 80%-ES6 or whatever it is that the version of node you are using in production can process? That way you don't have to update the code as node is able to use more ES6 features either, and other projects don't have to upgrade their node in lockstep with your project.

[inukshuk]

Do you use the extended date objects of this library or just the parser? I'm not sure if it's even possible to transpile the former to ES5, because the whole premise of the extended Date object is that it extends the built-in Date; if you want to support old browsers we may need to build a parser-only bundle.

[johanneswilm]

Just the parser. When you say extend you mean that it modifies how the built-in Date object works? Hopefully not. :-D.

For me personally it would be OK if you leave what you have now on "main" and then add an ES6 module version under "module". Then rollup/webpack will simply use the ES6 module version and we can adjust our bundler to make the right decisions on how much to turn into ES5. The tree shaker should leave out everything besides the parser given that we only use the parser. But I see that ORCID requires IE10 support and if they are switching to our bibtex parser, then that may not be enough for them because babel seems to not be able to translate all of the edtf parser code to IE10 compatible ES5 code.

[retorquere]

Another way to import would be import edtf = require('edtf'), which is the way I tell typescript (and that would mean also babel I think) to import a commonjs module.

[johanneswilm]

I use the ES6-style import syntax for all imports, no matter whether they come from CommonJS or ES6. So I usually don't even have to know what kind of syntax they are written in. The bundler figures that out for me. I believe in the case of edtf it just gets confused because it sees nothing declared as "module", so the bundler thinks it must be ES5 of some kind and takes "main" instead. But then that is ES6 mixed with commonjs modules. And that's when it breaks.

With your recent changes, it actually works for rollup at least to run in Chrome. But then I noticed that even though I specify to the rollup-plugin-babel that I need IE10 support, it doesn't quite give me that for EDTF, nor does it give me an error when bundling, so it's just something I have to discover myself later on.

If I understand the "module" proposal [1] that has turned into a convention correctly, the idea is to kind of use "module" for ES6 and "main" for (transpiled) ES5. Could EDTF not just follow that?

Even if you leave the "main" as it is now in its "mixed" state, for the reasons you have to have it that way, it will be less of a problem for ES6-based tools as they will simply ignore the "main" entry anyway as long as there is a "module" entry.

[1] https://github.com/dherman/defense-of-dot-js/blob/master/proposal.md

[inukshuk]

OK, I'll look into using ES6 modules with a native module loader (because they are beginning to emerge now) and to include a CommonJS variant for compatibility. We'll have to make sure the parser can be imported in isolation (tree shaking or otherwise), because it should be possible to transpile everything to ES5 for old browsers. I doubt this would work for some of the features used for the date extensions (obviously we're not modifying the built-in, that's the whole point of being able to extend it and the main reason was written in ES6 from the start), but just the parser should work.

[johanneswilm]

Yes, so in theory, given that I only use the parser and that rollup does tree-shaking, it should only include those parts that can be transpiled to ES5. Yet somehow that didn't work out. Unfortunately, it still seems to take forever to set up a working test/bundler setup, so to save some time, you may want to look into what we already have ste up in the biblatex-csl-converter package. Just run npm run compile_gh-pages, then serve the folder docs/ and open in a browser like IE11 and check the error messages. Adjust .babelrc and the rollup config file to try other configurations.

You may have more of an idea what is stopping it. You'll probably want to remove the exclusion of "node_modules" from the babel configuration.

[inukshuk]

Here is something you could try in the meantime: import edtf from 'edtf/src/parser' and use edtf.parse -- this is just an idea, without knowing exactly how RollUp would resolve this, but this may be a way to import just the parser code which, I would think, should transpile down to ES5 fine.

[johanneswilm]

I spent about two hours on trying this and it wasn't working. I suspect it is because I needed to allow babel to transpile modules in node_modules, but this is in turn upsetting rollup as it gets confused when the modules inside node_modules are transpiled. In the end it all comes down to an ES6 module being specified as "main". I don't think it makes much sense for me to continue with this right now. Let me wait until you have reorganized your npm listing as that will likely fix most or all issues.

[johanneswilm]

Btw, replacing:

-import edtf from 'edtf'
+import edtf from 'edtf/src/parser'

Makes the package 75kb smaller. Functionality seemingly being the same.

[johanneswilm]

@inukshuk The last change creates more problems when trying to include the package built from this in other packages and trying to pack those with webpack or rollup. browserify does not seem to mind. Webpack gives this error:

Module not found: Error: Can't resolve 'edtf/src/parser' in '[...]/node_modules/biblatex-csl-converter/src'

[johanneswilm]

rollup does not like import edtf from 'edtf' either. So I guess I cannot really use this until the edtf.js package has been fixed.

[johanneswilm]

I fixed the issue temporarily by replacing the edtf parser with a simpler LGPL licensed parser that I quickly threw together to cover all the parts we were using so far. It is good enough to cover our test cases. In the meantime our bundle shrunk by 37.5%. See fiduswriter/biblatex-csl-converter@2f06b29 Hopefully we can soon replace it with a fixed edtf.js parser package, but it now less urgent. @retorquere @inukshuk

[retorquere]

Where is this parser active? On parse of the biblatex, or on generation of the output?

[johanneswilm]

Both. It verifies that the date string is an EDTF L0/1 date format when parsing bibtex. It also uses it to turn the date string into the csl format on export.

[retorquere]

But what does it do when it decides a date is not EDTF L0/1? I mean will import behavior change between this date detector and EDTF.js?

[johanneswilm]

If there is no date field, it tries to create one with existing year and month fields. It doesn't check the contents of those fields, just joins them together ${year}-${month} or just the year depending on what is available. If either the original date field or this string gives a valid edtf L0/1 string, it accepts it and stores it in the date field. Otherwise it treats it like a badly formatted field. I have not changed how it behaves, but I can see that maybe we need some more complex parsing of cases like "2012 [1984]".

[johanneswilm]

It does also try to convert month names in case there is a month field. Nothing has changed about this. it would just be good to remove the edtf code again and use this library so we don't have to maintain it.

[retorquere]

Alright, steady as she goes then.