Feature/specify import require paths #1941

GreenImp · 2020-08-07T00:15:00Z

What it changes

There are three main parts of this PR:

Add .js extension to all the import statements in the src directory.
This makes it ES module compliant, as file extensions are required for ESM, and work for CommonJS as well.
When scripts are compiled to CommonJS or ESM it retains the file extension in the import / require statements.
I did try to get Gulp to be able to change the extension to .mjs for ESM and .cjs for CommonJS, but couldn't find a way of doing it.
Instead, I had to settle with adding a package.json file in the compiled ESM directory to specify the type: module, the inverse of the example here: https://2ality.com/2019/10/hybrid-npm-packages.html#variant%3A-avoiding-.mjs It can't be a bad idea if he's suggesting it 😉
Adds conditional exports property to the packages.json file, which tells node 12+ which files to use, depending on whether you're using import or require.
On node < 12, it will fallback to the main property of the package.json as it currently does, so there should be no difference there.
Changes the Gruntfile so that all compiled scripts are now under a single root directory, lib, rather than several different ones.
We now have:
- /dist -> /lib/browser/: The scripts suitable for browser use
- /lib -> lib/umd/: The CommonJS / UMD scripts
- /esm -> /lib/esm: The ES module scripts

It also removes the root /number.js file, as the exports points directly to the compiled ones.

What it fixes

This aims to fix #1928 when trying to use mathjs ESM directly in node, without a package bundler.
Currently, node either tries to load the CommonJS versions and doesn't like it or, if you specify an ESM script it complains about the lack of file extension.

It also attempts to fix the issue #1766 although there's currently an outstanding issue with Jest, where it doesn't understand the exports property.
However, once Jest does support exports, these changes should work for Jest as well, fixing that issue.
In the meantime, you should be able to work around it by being specific with the import path:

import mathjs/lib/esm/number.js should work as the package.json tells us it's an ESM file.
import mathjs/lib/umd/number.js should also work as it uses CommonJS require anyway.

How it's tested

I have made sure that npm run build still completes successfully, and that the tests all still run and pass (locally, and they also pass in Travis).
I've tested the changes manually in a separate project, by creating both an .mjs and .cjs file, importing mathjs and running scripts.
I've also run it through a bundler (Rollup) and it all still works.
The automated Travic tests seem to assert that Webpack handles things correctly.

These changes are very much breaking changes.

I'm more than happy to discuss this PR, and happy to make any changes.

Using `prepare` step to build ensures that dependency can be installed directly from git repository https://docs.npmjs.com/misc/scripts#prepublish-and-prepare `prepublishOnly` is run **after** `prepare` script so this should have no effect. Co-authored-by: Jos de Jong <wjosdejong@gmail.com>

* chore(package): update @babel/core to version 7.8.7 * chore(package): update @babel/preset-env to version 7.8.7 * chore(package): update lockfile package-lock.json Co-authored-by: greenkeeper[bot] <23040076+greenkeeper[bot]@users.noreply.github.com>

…#1774) * remove contributors from package.json in favour of AUTHORS The contributors section of our package.json is _very_ long and manually updating can (?) be a pain. I took a leaf out of nodejs's book and copied their update-authors script [1]. [1]: https://github.com/nodejs/node/blob/master/tools/update-authors.js * remove some of my many emails * add a couple more mail mappings * get lint passing * add instruction to update-authors Co-authored-by: Jos de Jong <wjosdejong@gmail.com>

…ecision when input contains BigNumbers

* make dot product faster and correct for ℂ * minor fixes * added tests, fixed bugs * add dot for sparse matrices * make multiply(vec, vec) use dot * add test for complex vectors * added test for mul(vec, vec), removed one TODO comment Co-authored-by: Jos de Jong <wjosdejong@gmail.com>

Co-authored-by: Jos de Jong <wjosdejong@gmail.com>

…ong#1055

Shouldnt provide any issues for other systems I hope

This reverts commit 4cd2704.

This reverts commit 268b594.

Bumps [@babel/preset-env](https://github.com/babel/babel) from 7.9.0 to 7.9.5. - [Release notes](https://github.com/babel/babel/releases) - [Changelog](https://github.com/babel/babel/blob/master/CHANGELOG.md) - [Commits](babel/babel@v7.9.0...v7.9.5) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

* Prefix the cli test with 'node' so it works on windows Shouldnt provide any issues for other systems I hope * Revert "Prefix the cli test with 'node' so it works on windows" This reverts commit 4cd2704. * Revert "Revert "Prefix the cli test with 'node' so it works on windows"" This reverts commit 268b594. Co-authored-by: Jos de Jong <wjosdejong@gmail.com>

…ejong#1811)

GreenImp · 2020-08-22T23:36:51Z

I've had a play, trying to reproduce that node issue you mentioned in point 10:

$ node .\test.mjs
import { sqrt } from 'mathjs'
         ^^^^
SyntaxError: The requested module 'mathjs' is expected to be of type CommonJS, which does not support named exports. CommonJS modules can be imported by importing the default export.

I'm unable to reproduce the issue.
I've tried running your files that you linked above, and they seem to be fine.

I've tested in Node v11.15, v12.18, and v14.3
Which nodeJS version are you using?
Were you definitely running the local mathjs with the new package.json exports property?

josdejong · 2020-08-23T10:17:35Z

Thanks for your updates!

So if I'm not mistaken these points are still open to be adressed:

(1) I think we should really remove the non-minified file from bundle. I can do that if you want after merging your PR.
(2) backward compatibility console.log messages

Feedbacks on the other points:

(6) I think the docs where indeed already good, though now some undocumented cases that where not working before are just working now :)
(8) Mocha just supports ESM, see https://mochajs.org/#nodejs-native-esm-support
Currently, to test one file, you can run the following, where you have to specify that the files must be transpiled with Babel:
```
mocha test/unit-tests/utils/array.test.js --require @babel/register
```
This takes 3.1 seconds on my machine.

When we specify "type": "module" in package.json, and change the imports in the file array.test.js to be valid paths (rename '../../../src/utils/array' to '../../../src/utils/array.js'), we can run the test as follows, without need for transpilation:
```
mocha test/unit-tests/utils/array.test.js
```
This takes 0.8 seconds on my machine.

Not having to do transpilation before being able to run unit tests will make the unit tests much faster (as we already see from this simple example).

There is an issue though since we when we change "type": "commonjs" to "type": "module", we cannot run nodejs with commonjs anymore (for example try to run node examples/basic_usage.js). Alternative could be to rename the test files to *.mjs. Do you know if it is possible to explain to node.js that it should pick the commonjs index instead when "type": "module" is defined? That would be the best of both worlds I guess.
(9) Good point. I think the most important task for the examples is to explain people how to use mathjs. If the examples do not work out of the box on older versions of node.js, it doesn't help. So maybe it's indeed best to leave the examples as it is for now.
(10) Thanks for checking. I'm running the latest node.js, v14.8.0. I just cleared all local stuff, checked out your branch again clean, built everything, and now the code examples I tried just work! I suppose it was indeed something vague on my end, probably an issue between the keyboard and the chair ;).

GreenImp · 2020-08-23T20:53:11Z

(1) and (2) Correct! They are outstanding. If you're happy to do point 1 after the PR has been merged, that would be really appreciated. And I'll look into point 2 now.
(8) That's a good speed increase!
This PR sets the packages type property to commonjs. This is mainly because the gulpfile is written in commonjs and Gulp has no understanding of ESM. I tried setting the "type": "module" and renaming the gulplfile to gulpfile.cjs, but Gulp will only look for a .js extension and there's no way to change it (I can't find e link now, but the official devs aren't planning on supporting it).
I think a reasonable solution would be to add a package.json file inside the tests directory, like we've got in the /lib/esm/ directory, that tells node it's ESM. That seems to work.
```
{
  "type": "module"
}
```
(10) I'm glad that's working for you now!

josdejong · 2020-08-29T11:07:35Z

(1) ok I'll address removing the non-minified bundle right after merging this PR (to prevent any merge conflicts)
(2) would be great if you can pick that up backward compatibility messages, thanks!
(8) I tried out creating a nested package.json in the test folder but it looks like mocha doesn't play nice with that.

I have the idea that I'm misunderstanding somthing: when I set "type": "module" in the root package.json, we would fix the mocha issue. But then I'm not able to load the library in a regular nodejs+commonjs setup, do you have the same? I would expect that it should be possible to get this combination working. I would love to make ESM first class, and CJS second class, but all should work out of the box with commonjs too.

Besides that we have to make the right, future-ready decision on the type in package.json, I think we can refactor the test into ESM separately from this PR to keep things focused. Unless you're in for doing that too straightaway :).
(11) Just another idea on this whole matter: we could also consider publishing two libraries: mathjs and mathjs-es (that's for example what Lodash does but even that can give issues, see this topic: How to use lodash-es using import in Node 14? lodash/lodash#4800). That is not ideal, but if ESM and CommonJS are biting each other too much this at least is a possibility.

GreenImp · 2020-09-03T09:49:37Z

Hey, sorry, I've not had the chance to get back to this for a while.

(1) Fab!
(2) Which files do you want to have the messages?
(8) That's odd, I'm sure I had that working. I'll try again and see.

Setting "type": "module" in the root package.json should work fine for both ESM and CommonJS (The "exports": {...} property tells node where to look). What issue are you seeing?

The problem that I had, was that Gulp doesn't seem to be compatible with ESM projects, because the gulplfile is written in CommonJS, but has a .js extension - if the type is module, node expects .js to be ESM.
(11) I think splitting into two libraries could cause confusion, with people installing the wrong one, and potentially increase maintenance.

GreenImp · 2020-09-04T12:22:54Z

Okay, I've re-checked point 8, and you're right it does have issues, using the nested package.json in the test directory.

Firstly it doesn't like the lack of extension in the imports. So I added them.
But then it doesn't like that the src directory uses ESM, as the library is still declared as CommonJS.
So I tried adding a package.json in the src directory, with "type": "module". But it then complains as some of the source files seem to use CommonJS style exports:

e.g. in src/bundleAny.js:

module.exports = /* #__PURE__ */ defaultInstance

If you set the root package.json to "type": "module", you're correct that CommonJS won't work with the code in this PR; you'll need to add a package.json file to the lib/cjs` directory like:

{
  "type": "commonjs"
}

Currently it's the other way around, and we have one in the lib/esm directory.

I do agree that it would be good to get it so that ESM is the "default" and set in the root package.json file. I think the only thing stopping that is the fact that the gulp process breaks.

josdejong · 2020-09-06T07:11:35Z

Hey, sorry, I've not had the chance to get back to this for a while.

No problem at all! Take it easy. I guess we're in the same boat 😉

(2) It would be good to replace the contents of the deleted index files with a console.error explaining what's going on and what to do:
```
main/es5/index.js
main/es5/number.js
main/esm/index.js
main/esm/number.js
number.js 
```
(8) Ahhh, of course! Having "type": "module" in the root package.json, and "type": "commonjs" in a new file /lib/cjs/package.json folder works for regular node.js. That makes sense. I've tested it on node 14, 12, and 10, and in all cases the "old" commonjs import works, and the esm import works on 12 with flag and 14 without flag.

I would love to go for an ESM first approach. Do you see any drawbacks/issues in going for "type": "module" in the root package.json? What is your take on this?
(11) I agree. And as far as I can see now the hybrid solution that you've worked out works like a charm 👍
(12) I think it would be good to write a few tests to validate that all the entries work, both for ESM and CJS, and with full library or just plain number support. I can look into that if you want.

josdejong · 2020-09-06T08:29:15Z

FYI: I've changed the base of this PR to be merged into v8 (next breaking version) instead of develop. See #1960.

GreenImp · 2020-09-06T16:40:16Z

(2) Thinking about this; those files aren't in the exports list in the package.json file, so if you try to import any of them, node will throw an error like:

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './main/es5/index.js' is not defined by "exports" in /var/www/mathjs-test/node_modules/mathjs/package.json imported from /var/www/mathjs-test/test.mjs

Except for math/number (Without the extension), which will still work, as the exports points to the lib version.

If you want the console.errors, the files will have to be added to the exports list, which I'm happy to do. Although I wonder if the error message above is enough of a hint, if it's explained somewhere in the docs?
(8) The only downside I can see is the the gulp build doesn't work. If the gulpfile can be written using ESM (There's lots of require - not sure if they can be replaced with import, or if Gulp will run it if you do), or if it can be replaced with an ESM compatible bundler (e.g. Webpback, Rollup), then I think it would be a good idea to move to an ESM first approach.
(12) To be honest, I've not really looked at the tests, other than ensuring that they still pass. So I'd appreciate it if you could take a look at that.

GreenImp · 2020-09-06T16:43:09Z

I've just found this: https://gulpjs.com/docs/en/getting-started/javascript-and-gulpfiles/
And this: gulpjs/interpret#65

Which suggests that Gulp does support ESM, if the gulpfile is named as gulpfile.esm.js. But there's no way of specifying that it's commonjs, inside an ESM library.
I'll have a play and see if I can get an ESM gulpfile working.

josdejong · 2020-09-06T17:53:22Z

(2) It's good that there is a clear error, but I prefer putting a bit of effort on our side to give an error message which also explains the user what to do, so the user doesn't have to google to figure out what's going on (it's frustrating enough already when upgrading a library has breaking changes). So yes, please add those 5 files temporarily to the exports list and create a console.error in the files offering an explanation.
(8) good point about the gulpfile, I hadn't thought about that yet. I expect this is solvable. Would be great if you can give it a try, and if you're stuck I can give it a try too. I think it's worth it to go ESM first now.

Is it maybe possible to rename the gulpfile to gulpfile.cjs, and change the package.json scripts like:
```
"build": "gulp",
```
such that we pass this file explicitly:
```
"build": "gulp --gulpfile gulpfile.cjs",
```
? Then we can postpone refactoring the gulpfile to ESM until later (just like the unit tests).
(12) 👍 I'll have a look into that after merging your PR.

GreenImp · 2020-09-06T20:29:35Z

(2) Okay, will do.
(8) Okay, that's new to me! I couldn't find the CLI arguments in the gulp docs, but just found the readme on the Github repo. Wish I'd seen that several weeks ago!
I've tried that out and it works like a charm. I'll make changes to have it ESM first..

GreenImp · 2020-09-06T22:28:46Z

I've got this mostly working when making the root package.json "type": "module".

I'm having issues with the src/bundleAny.js file, because it's written in CommonJS.
I've tried changing the extension to .cjs with some success, and I can then successfully build everything. But the tests now fail, because bundleAny uses require to import ES Modules:

Must use import to load ES Module: /var/www/mathjs/src/factoriesAny.js
require() of ES modules is not supported.
...
Instead rename factoriesAny.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /var/www/mathjs/package.json.

So I tried changing it to use import, as I thought node supported import of an ESM from a CJS file, but alas:

SyntaxError: Cannot use import statement outside a module

However, on my test project that includes the local repo, I can run both ESM and CommonJS without issues. So it looks like it's only a problem with the tests.
If we can et the bundleAny file converted to ESM, then that will probably fix the issue, but I'm not certain how to replicate the export it has, and your comment there seems to suggest you had similar issues:

// TODO: not nice having to revert to CommonJS, find an ES6 solution

josdejong · 2020-09-07T18:16:26Z

Ai! I see the problem. If I remember well I didn't find a solution to export dynamic output using ES6, or maybe it was that I couldn't get treeshaking working.

Let me give it a try finding a solution for this. I'm amazed that the library itself indeed does work despite this single commonjs file, only the build/test seem to have issues.

josdejong · 2020-09-07T18:16:43Z

I expect to be able to look into it coming Wednesday.

josdejong · 2020-09-07T18:27:17Z

I have an idea to turn this into a proper ES file: generate all the exports explicitly. Will require a small new build script similar to those other scripts generating index files etc, shouldn't be hard. Let me try that out coming Wednesday!

josdejong · 2020-09-07T21:04:34Z

I now understand why the library works but the tests don't: the bundleAny.js file is only used in tests and to generated the browser bundle, it's not used in the ES or CJS output.

josdejong · 2020-09-09T12:41:38Z

It was quite a puzzle but I think I've got everything working.

I've merged this PR into a new branch in the mathjs repo and created a PR: #1962

Still a few open ends, but getting there 😅

GreenImp · 2020-09-09T13:31:13Z

That's fantastic! Looks good.
Does this mean that this PR is no longer necessary?

josdejong · 2020-09-09T13:35:10Z

Your PR is merged only this page doesn't yet know it because your work is still in a separate feature branch. I expect it to be marked as "merged" by Github automatically once we've merged this feature branch.

* Add `.js` extension to source file imports * Specify package `exports` in `package.json` Specify package type as `commonjs` (It's good to be specific) * Move all compiled scripts into `lib` directory Remove ./number.js (You can use the compiled ones in `./lib/*`) Tell node that the `esm` directory is type `module` and enable tree shaking. Remove unused files from packages `files` property * Allow importing of package.json * Make library ESM first * - Fix merge conflicts - Refactor `bundleAny` into `defaultInstance.js` and `browserBundle.cjs` - Refactor unit tests to be able to run with plain nodejs (no transpiling) - Fix browser examples * Fix browser and browserstack tests * Fix running unit tests on Node 10 (which has no support for modules) * Fix node.js examples (those are still commonjs) * Remove the need for `browserBundle.cjs` * Generate minified bundle only * [Security] Bump node-fetch from 2.6.0 to 2.6.1 (#1963) Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1. **This update includes a security fix.** - [Release notes](https://github.com/bitinn/node-fetch/releases) - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md) - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> * Cleanup console.log * Add integration tests to test the entry points (commonjs/esm, full/number only) * Create backward compatibility error messages in the files moved/removed since v8 * Describe breaking changes in HISTORY.md * Bump karma from 5.2.1 to 5.2.2 (#1965) Bumps [karma](https://github.com/karma-runner/karma) from 5.2.1 to 5.2.2. - [Release notes](https://github.com/karma-runner/karma/releases) - [Changelog](https://github.com/karma-runner/karma/blob/master/CHANGELOG.md) - [Commits](karma-runner/karma@v5.2.1...v5.2.2) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> Co-authored-by: Lee Langley-Rees <lee@greenimp.co.uk> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

josdejong · 2020-11-06T11:03:27Z

I've just published v8.0.0, thanks again for all your work Lee!

This PR should be marked as merged already, I'm not sure why it doesn't. Will close the PR now, all your code is in master.

josdejong and others added 30 commits February 26, 2020 11:16

Fix examples in documentation of row and column, see josdejong#230

52f6def

Publish v6.6.1

033a6ba

Create a test:all script to remove duplication (see josdejong#1751)

9272395

Update history and contributor list

6389ed1

Update devDependencies

13ce295

remove trailing space in HISTORY (josdejong#1775)

d8d9a93

Update devDependencies

1dbae6e

Update devDependencies

b2af1d7

Fix josdejong#1789: Function eigs not calculating with BigNumber pr…

5624e69

…ecision when input contains BigNumbers

Publish v6.6.2

1496cc5

Update history

56e0d1e

Update devDependencies

b8f2636

Fixed eigs not using config.epsilon (see josdejong#1789)

7e8d128

Should be able to take NAMED_DELIMITER as object keys (josdejong#1798)

fae6527

Co-authored-by: Jos de Jong <wjosdejong@gmail.com>

Update history

a23f9c3

Clarify the documentation on scope when using rawArgs, see josdej…

3153de2

…ong#1055

Prefix the cli test with 'node' so it works on windows

4cd2704

Shouldnt provide any issues for other systems I hope

Revert "Prefix the cli test with 'node' so it works on windows"

268b594

This reverts commit 4cd2704.

Revert "Revert "Prefix the cli test with 'node' so it works on windows""

6bff420

This reverts commit 268b594.

Merge branch 'develop' into Undo

76a9506

Fix josdejong#1808: toNumber() not working on a unitless unit (josd…

7833e23

…ejong#1811)

Update history

9f8564b

Diff function

bc3cbbc

Included createDiff in factoriesAny

49a6521

josdejong mentioned this pull request Sep 6, 2020

Breaking changes for v8 #1960

Closed

4 tasks

josdejong changed the base branch from develop to v8 September 6, 2020 08:28

Make library ESM first

d98fb11

josdejong mentioned this pull request Sep 9, 2020

Specify import require paths (continuation of #1941) #1962

Merged

7 tasks

josdejong changed the base branch from v8 to experiment/operators_with_node_support September 9, 2020 13:33

josdejong mentioned this pull request Sep 16, 2020

TypeError: Cannot read property 'prototype' of undefined when run in jest #1898

Open

rev111 mentioned this pull request Oct 15, 2020

Error: math.import is not a function #1997

Closed

josdejong closed this Nov 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/specify import require paths #1941

Feature/specify import require paths #1941

GreenImp commented Aug 7, 2020 •

edited

Loading

GreenImp commented Aug 22, 2020

josdejong commented Aug 23, 2020

GreenImp commented Aug 23, 2020 •

edited

Loading

josdejong commented Aug 29, 2020

GreenImp commented Sep 3, 2020

GreenImp commented Sep 4, 2020 •

edited

Loading

josdejong commented Sep 6, 2020

josdejong commented Sep 6, 2020

GreenImp commented Sep 6, 2020

GreenImp commented Sep 6, 2020

josdejong commented Sep 6, 2020 •

edited

Loading

GreenImp commented Sep 6, 2020

GreenImp commented Sep 6, 2020

josdejong commented Sep 7, 2020

josdejong commented Sep 7, 2020

josdejong commented Sep 7, 2020

josdejong commented Sep 7, 2020

josdejong commented Sep 9, 2020

GreenImp commented Sep 9, 2020

josdejong commented Sep 9, 2020

josdejong commented Nov 6, 2020

Feature/specify import require paths #1941

Feature/specify import require paths #1941

Conversation

GreenImp commented Aug 7, 2020 • edited Loading

What it changes

What it fixes

How it's tested

GreenImp commented Aug 22, 2020

josdejong commented Aug 23, 2020

GreenImp commented Aug 23, 2020 • edited Loading

josdejong commented Aug 29, 2020

GreenImp commented Sep 3, 2020

GreenImp commented Sep 4, 2020 • edited Loading

josdejong commented Sep 6, 2020

josdejong commented Sep 6, 2020

GreenImp commented Sep 6, 2020

GreenImp commented Sep 6, 2020

josdejong commented Sep 6, 2020 • edited Loading

GreenImp commented Sep 6, 2020

GreenImp commented Sep 6, 2020

josdejong commented Sep 7, 2020

josdejong commented Sep 7, 2020

josdejong commented Sep 7, 2020

josdejong commented Sep 7, 2020

josdejong commented Sep 9, 2020

GreenImp commented Sep 9, 2020

josdejong commented Sep 9, 2020

josdejong commented Nov 6, 2020

GreenImp commented Aug 7, 2020 •

edited

Loading

GreenImp commented Aug 23, 2020 •

edited

Loading

GreenImp commented Sep 4, 2020 •

edited

Loading

josdejong commented Sep 6, 2020 •

edited

Loading