add commonjs entrypoint for nodejs environment #1928

Merged
merged 11 commits into from Oct 27, 2016

Projects

None yet

5 participants

@iamstarkov
Contributor
iamstarkov commented Oct 6, 2016 edited

i discussed it in gitter and got green light to try out this approach.

So basically there is no need to bundle all the files into one for nodejs environment, so we can export natively supported one entry point which requires all internal files.

One instant benefit is clearer stack traces:

➜  projects node
> const old = require('./ramda-old');
undefined
> const R = require('./ramda');
undefined
> old.pipe()
Error: pipe requires at least one argument
    at Object.pipe (/Users/vlasta/projects/ramda-old/dist/ramda.js:7421:19)
    at …
> R.pipe()
Error: pipe requires at least one argument
    at Object.pipe (/Users/vlasta/projects/ramda/src/pipe.js:32:11)
    at …
> 

I didnt touch anything else: neither testing, nor building processes.

This pr has aslo benefit of possible smaller builds for smart bundlers (without hacks relying on frozen file system structure).

Pros:

  • clear stacktraces
  • smaller builds
  • room for improvements in developer experience (faster tests, simpler build, windows friendly setup, etc)

No cons.

@iamstarkov iamstarkov add commonjs entrypoint for nodejs environment c13b972
@iamstarkov
Contributor

why do you want to generate it each release?

its kind of contract for outer world which functions ramda exposes on top level

Its need to be updated only if:

  1. new function added
  2. deprecated function have to be removed.
  3. new function is added => relevant tests needed =>require it from .. => if its undefined add function to the list. So your tests will work => list of function will always be up to date.
  4. removing functions wo/ removing them form the list can be addressed either by sanity test that verifies every exported function is a function indeed, either by build process which fails if required file doesnt exist
@iamstarkov
Contributor

tldr:

  • ./index.js doesnt need to be updated with each release
  • it needs to be updated every time api surface changes, but its the only one case
@davidchambers
Member

either way -- at release time or at add/remove time -- we need an automated process to update index.js. Agreed?

I do not agree. Adding an entry to index.js when adding a new function is not onerous, and the test suite can ensure that this step is not forgotten.

@davidchambers
Member

and the test suite can ensure that this step is not forgotten.

Is the test suite not an automated process?

It's an automated process, but not "an automated process to update index.js". 😜

Does the test suite do this today?

No, but we don't have index.js yet.

If not, someone would have to create such a test that would run at add/remove time?

I'm not suggesting one test per function. I'm suggesting something like this:

eq(Object.keys(require('..')).sort(),
   Object.keys(require('../index.js')).sort());

Were someone to open a pull request to add R.quux without adding a 'quux' entry to index.js, 'quux' would appear in the first array but not in the second, and we'd detect the omission.

@iamstarkov
Contributor

either way -- at release time or at add/remove time -- we need an automated process to update index.js. Agreed?

disagreed. dont need to do that on release time, because index.js should be up to date whenever release needs to happen

@iamstarkov
Contributor

My main objection against generated index.js — it makes things much more complicated:

  • you have to be keep it out of repository
  • tests are broken by default, because there is no index.js in start
    and so on
@iamstarkov
Contributor

compromise can be to replicate something like require-dir, but its will work only until es6 modules will landed in nodejs, because then modules' exports have to be static.

@iamstarkov
Contributor
iamstarkov commented Oct 8, 2016 edited

@davidchambers i also was thinking about the same simple test

@iamstarkov iamstarkov add section about removing/adding to contributing guide cc92c06
@iamstarkov
Contributor

added

  • one more step in contributing guide regarding changing API surface.
  • test case for it
@iamstarkov iamstarkov add test for consistent api surface a8c3f4c
CONTRIBUTING.md
@@ -12,17 +12,20 @@
of the following release. If adding an internal function `_foo`, define it
in __src/internal/_foo.js__.
-4. Make one or more atomic commits. Each commit should have a descriptive
+4. If adding new functions or removing existing ones, consider to update
+ `./index.js` exports list with corresponding changes.
@davidchambers
davidchambers Oct 8, 2016 Member

This could be included in step 3:

If adding a function `R.foo`, define it in __src/foo.js__, require it in __index.js__, and include unit tests in __test/foo.js__.
                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

We needn't document the process for removing functions as this is only ever done by members of the core team.

@iamstarkov
iamstarkov Oct 9, 2016 Contributor

understood

@iamstarkov
iamstarkov Oct 9, 2016 Contributor

updated

@iamstarkov iamstarkov update contributing guide 8696b29
test/index.js
+ var removeJsEnding = function(file) { return file.replace('.js', ''); }
+
+ var exportedApi = Object.keys(R);
+ var actualApi = fs.readdirSync(srcFolder).filter(isJsFile).map(removeJsEnding);
@davidchambers
davidchambers Oct 9, 2016 Member

Why use fs.readdirSync? Couldn't we simply compare require('../index.js') with require('..')?

@iamstarkov
iamstarkov Oct 9, 2016 Contributor

isnt it the same?

@iamstarkov
iamstarkov Oct 9, 2016 edited Contributor

i mean require('..') points to a parent folder with package.json, which points to ../index.js, so require('..') === require('../index.js')

@iamstarkov
iamstarkov Oct 9, 2016 Contributor

with readdirSync i check if exported api surface matches all the files in ./src/ folder

@davidchambers
davidchambers Oct 11, 2016 Member

What is the purpose of the test? I would say it's to verify that require('ramda/index.js') and require('ramda/dist/ramda.js') are equivalent. Note that I did not mention the locations of files other than index.js and dist/ramda.js.

The test you've provided asserts this equivalence, but indirectly. It forces us to make assumptions about the project layout. Were we to add src/somePrivateFunction.js our assertion would fail because R would not have a property named somePrivateFunction.

@iamstarkov
iamstarkov Oct 11, 2016 Contributor

at this point i have two questions:

  • how, how often and what is the process of commiting new dist files?
  • if contributor opens a pull-request with new function, does he need to include dist files in pull-request? if so, how do ramda team verify minified code in dist folder?
@iamstarkov
iamstarkov Oct 11, 2016 Contributor

What is the purpose of the test?

to verify exported API surface matches actual API surface in ./src/*.js. I assumed this part, because it looks like so, and anyway i need to find any actual source of truth.

I would say it's to verify that require('ramda/index.js') and require('ramda/dist/ramda.js') are equivalent.

I didn't want to rely on dist files, because dist API will be non synced with actual API in period between new feature landed and new version released. Here I assume that pull-request authors do not commit dist files, because dist are big and one of them is minified, so its hard to verify them.

@iamstarkov
iamstarkov Oct 11, 2016 Contributor

The test you've provided asserts this equivalence, but indirectly.

at first, it may seems so, but i'd argue with that, because if asserting actual files is indirect, then asserting result generated from those files is more indirect then first approach.

@davidchambers
davidchambers Oct 11, 2016 Member

what is the process of commiting new dist files?

  1. A member of the core team runs make release-minor (or make release-patch or, one day, make release-major).
  2. This expands to:
node_modules/.bin/xyz --repo git@github.com:ramda/ramda.git --script scripts/prepublish --increment minor
  1. xyz runs scripts/prepublish.
  2. The prepublish script runs the following commands:
rm -f dist/ramda{,.min}.js
make dist/ramda{,.min}.js
git add dist/ramda{,.min}.js
  1. xyz commits the changes, tags the commit, and pushes to GitHub and to the public npm registry.

I didn't want to rely on dist files, because dist API will be non synced with actual API in period between new feature landed and new version released.

The test target in the makefile depends on dist/ramda.js, so running make test will update the file if necessary. ;)

@iamstarkov
iamstarkov Oct 11, 2016 Contributor

It forces us to make assumptions about the project layout. Were we to add ./src/somePrivateFunction.js our assertion would fail because R would not have a property named somePrivateFunction.

this constraint you mentioned is legit, my two cents here are: since 0.9 release this assumption was true + all internal/private functions are keept in ./src/internal folder

@iamstarkov
iamstarkov Oct 11, 2016 Contributor

The test target in the makefile depends on dist/ramda.js, so running make test will update the file if necessary. ;)

👍 thats good and as far as i can see travis also runing make test, one downside of this is that windows-based contributors are not be able to contribute anymore

@iamstarkov
iamstarkov Oct 11, 2016 edited Contributor

other side note is: npm test also runs npm run build, which in turns run make.

Every time you want to test your code, you need to run build, which in turns ruin fast tests. its kinda sad to me

@iamstarkov
iamstarkov Oct 11, 2016 Contributor

@davidchambers i can live without nice dx on windows, because it was so from first release.

@iamstarkov
iamstarkov Oct 11, 2016 Contributor

@davidchambers thanks for your explanation, now i agreed with you that we can check explicitly require('ramda/index.js') and require('ramda/dist/ramda.js'), so i'll update this pull-request

@iamstarkov
iamstarkov Oct 11, 2016 Contributor

though there is room for improvement for dx in overall (slow test process), and dx on windows (impossible now), but lets have another discussion about it 🙌

@iamstarkov iamstarkov update test for api surface f70850a
@iamstarkov
Contributor

@davidchambers updated pr

test/index.js
+
+describe('api surface', function() {
+ var exportedApi = Object.keys(R);
+ var actualApi = Object.keys(dist);
@davidchambers
davidchambers Oct 11, 2016 Member

We should .sort() each array before comparing them.

@iamstarkov
iamstarkov Oct 11, 2016 Contributor

sure, fixed

@iamstarkov iamstarkov test: sort both api arrays before asserting them 72c935d
@iamstarkov
Contributor

@buzzdecafe, hi, i realized you removed your comments. I'm worried. Did i do smth wrong?

@iamstarkov
Contributor

any further comments or this pr is ready for merge?

@buzzdecafe
Member

@buzzdecafe, hi, i realized you removed your comments. I'm worried. Did i do smth wrong?

no, not at all. i just wanted to withdraw from the conversation. Carry on.

@davidchambers
Member

I like this change. I don't think @buzzdecafe and @CrossEye are convinced, though.

@CrossEye
Member

I simply haven't been able to give it time. I should be able to do so in the next two days.

@CrossEye
Member

I don't have any objections. I can see the need.

But there definitely is a part of me that wants this generated in the build, and not a manual step to take. I won't fight for that, though. If I want to figure out how to do that, I'll worry about it.

@CrossEye
Member

🌿

@iamstarkov
Contributor

I see that PR is kinda stucked, maybe the reason is lack of communication from my side. To fix that let me tell you my story and motivation. Sorry, its gonna be a bit long.

I enjoy ramda and build websites, also i want to contribute.

Packing code which uses ramda lacks several bundle optimization due to ramda's entrypoint is being pointed to pre-built script. Its also a reason for a bit weird stacktraces like dist/ramda.js:7421:19.

"Ok, i can fix that"—i thought. "Lets fix the entry point, and write tests. Thats gonna be easy."

Eventually, i found a time to start working on this issue concerning me. I implemented it, though tests run 25s, where 14s is build time. This means few cons for me as a developer: very slow tests, impossible to use tdd as usual, and also automaticaly generated files in repo which are impossible to review and verify.

Usually people are .gitignore-ing those files and build them only right before publish, removing them right after publish. So those files are available via distribution channels like npm and CDNs, but not through GitHub.
I believe that generated files should be a result of transformation on actual files (bundling and minifying), so source files should not depend on dist files. Right now, and even with this pull-request its not the case.

I made short investigation and it turns out, that direct child files in src folder are used by current build setup and they are exported as top level api methods. That said, i made the same assumptions to rely on source files with the help of fs.readDirSync as initial PR implementation. It made me confident, that long entrypoint file will always be in sync with actual source files.

At this point in order to to make tests fast we already can separate build and test processes from each other. Almost.

Caveat here is that we need to make sure build process uses entry point (main field) from package.json to build dist files, to ensure that dist files are in sync with source files. Here things become complex.

Current build process scans direct child files in src folder and uses those files as top level api methods to export in dist files and does not pay attention to main field. Which is fine, until we want to do things i described. I looked into build script and couldnt find an easy way to make it do what we need. I tried another approach to use widely accepted bundle tools like webpack or browserify, but dist/ramda.js and dist/ramda.min.js became much bigger, so it was not worth it. Btw, kudos for awesome work here with build script!

I was thinking about merging this pull-request and later raise a concern about separating build && test processes.

My thought is if we want to verify that dist files are in sync with source files, we need either tune build script to consume main field and write tests for build script itself, or use browserify/webpack and find a way to tune them to make dist files as small as they are now.

In the middle of the pull-request we decided to rely on dist file in a test. This decision is blocking way to implement things i described. Though, I'm fine about it, and i hoped to discuss next steps in further issues/PRs.

I appreciate time and effort all of you spent on ramda. I will understand if you dont want this pull-request to be merged. After all of you are the core team, who needs to be comfortable with changes i'm introducing here.

In the end of the journey, i want ramda to be optimised for node as well as for browsers, separated build & test flows, better developer experience (dx).

Do you feel need in these goals to be implemented and do you feel comfortable with changes? If so, lets do it in one or another way. I assume my implementation vision can be imperfect.

@kedashoe
Contributor

Thank you for the thoughtful post and for sticking with this PR!

I pulled down your branch, it looks like we can reproduce the build using your new index.js file by changing https://github.com/ramda/ramda/blob/master/scripts/build#L262-L263

R.filter(R.test(/(^|[/]).*[.]js$/),
fs.readdirSync(path.join(__dirname, '..', 'src'))) :

to

R.keys(require('../index.js')) :

(This could be made a little faster by reading index.js as a string, parsing out the export object and then reading that as json.. feel free to tackle that now if you'd' like, but no big deal at this point I don't think.)

Running the build with --complete with each of those produces the same output for me.

I'm not sure I understand everything in your post, is the idea something like: tests run against index.js are slow, but if we can build dist from index, then we can run tests against dist to get faster tests and know that index.js is ok?

I am 👍 once we address your remaining concerns :)

@davidchambers
Member

Updating the build script to reference index.js is a great idea, Kevin!

@iamstarkov
Contributor

I pulled down your branch, it looks like we can reproduce the build using your new index.js file by changing https://github.com/ramda/ramda/blob/master/scripts/build#L262-L263

Thats cool.

I'm not sure I understand everything in your post, is the idea something like: tests run against index.js are slow, but if we can build dist from index, then we can run tests against dist to get faster tests and know that index.js is ok?

Almost, i want to get rid off build step in order to run tests

@iamstarkov
Contributor

@kedashoe i can verify, changes you suggested have no effect on dist files:

➜  ramda git:(feat/commonjs-for-node) mkdir build-comp

# Old Build
➜  ramda git:(feat/commonjs-for-node) ll dist
total 672
-rw-r--r--  1 vlasta  staff   289K Oct 24 11:52 ramda.js
-rw-r--r--  1 vlasta  staff    41K Oct 24 11:52 ramda.min.js
➜  ramda git:(feat/commonjs-for-node) mv dist build-comp/dist-old

# New build:
➜  ramda git:(feat/commonjs-for-node) ✗ ll dist
total 672
-rw-r--r--  1 vlasta  staff   289K Oct 24 11:54 ramda.js
-rw-r--r--  1 vlasta  staff    41K Oct 24 11:54 ramda.min.js
➜  ramda git:(feat/commonjs-for-node) mv dist build-comp/dist-old

➜  ramda git:(feat/commonjs-for-node) mv dist build-comp
➜  build-comp git:(feat/commonjs-for-node) ✗ tree .
.
├── dist-new
│   ├── ramda.js
│   └── ramda.min.js
└── dist-old
    ├── ramda.js
    └── ramda.min.js

2 directories, 4 files
➜  build-comp git:(feat/commonjs-for-node) ✗ diff dist-old/ramda.js dist-new/ramda.js 
➜  build-comp git:(feat/commonjs-for-node) ✗ diff dist-old/ramda.min.js dist-new/ramda.min.js

Thats awesome

@iamstarkov
Contributor

So, now we can skip build step for running tests, right?

@iamstarkov
Contributor

Now, its guaranteed that dist files are in sync with source files, once we build ramda.

@iamstarkov
Contributor
iamstarkov commented Oct 24, 2016 edited

that said, we can get rid of build step to run tests, though in case of someone will add or remove new features, someone needs to build ramda and commit result build to pull-request. It will be hard to review and so on.

I would suggest to not rely on build files, as we decided in this pull-request earlier (/cc @davidchambers). Because, "its guaranteed that dist files are in sync with source files, once we build ramda", so we can run all test files over source files.

The only thing prevents us to go further is file naming/placing convention.
As we can see prev and proposed build script are only looking into /src/*.js files. Can we take use that as a convention for ./test/index.js? @davidchambers @buzzdecafe @CrossEye

If so, we can use originally proposed approach for testing exposed API, take a look here a8c3f4c

@CrossEye
Member

As we can see prev and proposed build script are only looking into /src/*.js files. Can we take use that as a convention for ./test/index.js?

That seems reasonable to me.

I'm not going to try to dig through our history to confirm this, so it might be based on the memory of a doddering old man, but I believe we used to run tests against the source files when we first split them from a single file. The trouble was that we ran into an issue where those worked but when we built the combined file with them it didn't. I don't recall why. So for a while, we were running the tests twice, once on the source files and once on the combined version. That was never very satisfying, and at some point we stopped seeing a need for the duplication.

I'm just wondering if we want a test step that runs against only the source files, as well as a second step just for the (hopefully more rare now that testing can be done independently) complete build that runs the tests against the combined files. For Node, that's perhaps overkill. But for browser-based solutions that will often use the full package, it might be necessary.

@iamstarkov iamstarkov rewrite APIs test, add note about used convention and explanation 2caf8ca
+ */
+describe('API surface', function() {
+ var exported = Object.keys(R);
+ var actual = sourceMethods(path.join(__dirname, '..', 'src'));
@iamstarkov
iamstarkov Oct 24, 2016 Contributor

Previosly we sorted these arrays, and then just eqed them. While its simple, it makes it hard to identify problem parts, when arrays are 200+ items each.

I would suggest to go with two checks. First lengths check is sufficient. And second check is human friendly. Check it out:
screen shot 2016-10-25 at 12 58 35 am

+ * Convention is
+ * * Actual API—all `./src/*.js` files are top level API methods
+ * * Exported API—object in `./index.js` to be exported
+ * * Actual and exported APIs should be the same
@iamstarkov
iamstarkov Oct 24, 2016 edited Contributor

This paragraph is a way to get everyone know about convention we agreed on this thread

+ * 1st case is detected in first assertion, and detailed in second one
+ *
+ * 2st case doesnt need detection, because NodeJS will throw an error
+ * if you would attempt to require non existing file
@iamstarkov
iamstarkov Oct 24, 2016 Contributor

Explanation here gives some clue to developer who accidentally broke this test

iamstarkov added some commits Oct 24, 2016
@iamstarkov iamstarkov fix eslint issues 01f40ea
@iamstarkov iamstarkov remove "npm pretest", because you dont need to build ramda before tes…
…ts anymore
46d34f2
@iamstarkov iamstarkov keep eslint check, but in CI now 431bfc5
@iamstarkov
Contributor
  • adjusted test for main entry
  • removed npm run pretest
  • eslint check is still needed, moved it into CI
@iamstarkov
Contributor
iamstarkov commented Oct 25, 2016 edited

The trouble was that we ran into an issue where those worked but when we built the combined file with them it didn't. I don't recall why.

Do we have those issues reported in ramda's repo?

I'm just wondering if we want a test step that runs against only the source files, as well as a second step just for the (hopefully more rare now that testing can be done independently) complete build that runs the tests against the combined files. For Node, that's perhaps overkill. But for browser-based solutions that will often use the full package, it might be necessary.

For sure, its an issue to address.

First things is to cover build script with tests. Browserify or webpack are tested on their own. So we should do that as well.

Second thing is to enable browser testing in CI process. We can do that easily and I will be happy to submit PR regarding this

@davidchambers
Member

I'm going to let @CrossEye and @buzzdecafe make a decision on this. I have my hands full with various Sanctuary projects at the moment.

@iamstarkov
Contributor

@CrossEye, @buzzdecafe what do you think?

@buzzdecafe
Member

i haven't paid attention, so consider that tacit approval

@CrossEye
Member

🌿

@CrossEye CrossEye merged commit 7cbc510 into ramda:master Oct 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@iamstarkov iamstarkov deleted the iamstarkov:feat/commonjs-for-node branch Oct 28, 2016
@iamstarkov
Contributor
iamstarkov commented Oct 28, 2016 edited

🎉yay🎉
thank you

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