Skip to content
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

Switch to the Node 12 experimental module system #12

Closed
wants to merge 1 commit into from
Closed

Conversation

Avaq
Copy link
Collaborator

@Avaq Avaq commented Sep 29, 2019

Motivation

Node 12 has introduced breaking changes to the way that the experimental module system works. Users of experimental modules will expect packages to follow this new system going forward.

Changes

  • Package entry point change from index to index.mjs: Node 12 no longer switches transparently between the most appropriate index file. Every package must provide an explicit path to its entry-point.
  • Build output renamed to umd.js: User have to deep-require this file now, as the package entry point is a .mjs file. I think using a specific name like this is clearer than using index.js.
  • "Package type" set to commonjs: This is a way for Node 12 to understand what the type is of files that don't express the type in their extension. We use commonjs so that our .js files are treated as such, so that we keep backwards compatibility on those files with older versions of Node.
  • Rollup config file extension changed: Node 12 forbids require('*.mjs') calls. Since this is how, deep down, the file was being loaded by Rollup, we get an error in Node 12.

Tests

The tests in this module run through Mocha and nyc. Since Mocha and/or nyc use require to load the files, and Node 12 forbids require('*.mjs') now, the tests don't run in Node 12. I left this problem for another time. In Fluture, I switched to an approach where I run the tests natively using c8 and oletus.

@Avaq Avaq requested a review from dicearr September 29, 2019 09:02
@coveralls
Copy link

coveralls commented Sep 29, 2019

Pull Request Test Coverage Report for Build 203

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 201: 0.0%
Covered Lines: 264
Relevant Lines: 264

💛 - Coveralls

@Avaq
Copy link
Collaborator Author

Avaq commented Sep 29, 2019

Another approach is not to provide a umd build at all, and ask users to rely solely on esm. Yet another approach is to make the umd build the main file, and ask mjs users to deep-require import from 'monastic/index.mjs'.

@@ -2,15 +2,16 @@
"name": "monastic",
"version": "1.0.1",
"description": "A functional approach to stateful computations",
"main": "index",
"type": "commonjs",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should lint-package-json assert that "type" is specified?

@dicearr
Copy link
Owner

dicearr commented Sep 30, 2019

Now that rollup.config.mjs got renamed to roullup.config.js shouldn't we use commonjs inside?

@dicearr dicearr mentioned this pull request Jul 18, 2020
@dicearr dicearr closed this in #16 Jul 27, 2020
@dicearr dicearr deleted the avaq/node-12 branch November 4, 2020 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants