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

[CHORE]: Convert project to ESM #39

Open
dgrebb opened this issue Oct 23, 2023 · 5 comments
Open

[CHORE]: Convert project to ESM #39

dgrebb opened this issue Oct 23, 2023 · 5 comments
Assignees
Labels
bug Something isn't working chores Maintenance and other feature-unrelated tasks. enhancement New feature or request question Further information is requested

Comments

@dgrebb
Copy link
Owner

dgrebb commented Oct 23, 2023

Requirements

A number of dependencies are now 100% ESM, and do not work with CommonJS/require().

The project should be converted, entirely, to ESM. This conversion is backwards compatible with CommonJS, but CommonJS is not compatible with ESM.

p-map and chalk for example

p-map

❯ npm run sanity-test

> [email protected] sanity-test
> cd test/configs/ && node ../../cli/index.js test

/BackstopJS/core/util/createBitmaps.js:4
const pMap = require('p-map');
             ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /BackstopJS/node_modules/p-map/index.js from /BackstopJS/core/util/createBitmaps.js not supported.
Instead change the require of index.js in /BackstopJS/core/util/createBitmaps.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/BackstopJS/core/util/createBitmaps.js:4:14)
    at Object.<anonymous> (/BackstopJS/core/command/reference.js:1:23)
    at requireCommand (/BackstopJS/core/command/index.js:49:26)
    at Array.map (<anonymous>)
    at Object.<anonymous> (/BackstopJS/core/command/index.js:46:4)
    at Object.<anonymous> (/BackstopJS/core/runner.js:1:24)
    at Object.<anonymous> (/BackstopJS/cli/index.js:6:16) {
  code: 'ERR_REQUIRE_ESM'

Node.js v20.8.1

chalk

❯ npm run sanity-test

> [email protected] sanity-test
> cd test/configs/ && node ../../cli/index.js test

/BackstopJS/core/util/logger.js:1
const chalk = require('chalk');
              ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /BackstopJS/node_modules/chalk/source/index.js from /BackstopJS/core/util/logger.js not supported.
Instead change the require of index.js in /BackstopJS/core/util/logger.js to a dynamic import() which is available in all CommonJS modules.
    at Object.<anonymous> (/BackstopJS/core/util/logger.js:1:15)
    at Object.<anonymous> (/BackstopJS/core/command/index.js:3:16)
    at Object.<anonymous> (/BackstopJS/core/runner.js:1:24)
    at Object.<anonymous> (/BackstopJS/cli/index.js:6:16) {
  code: 'ERR_REQUIRE_ESM'
}

Node.js v20.8.1
@dgrebb dgrebb added bug Something isn't working enhancement New feature or request question Further information is requested chores Maintenance and other feature-unrelated tasks. labels Oct 23, 2023
@dgrebb dgrebb added this to the Node Support (>=18.0.0) milestone Oct 23, 2023
@dgrebb
Copy link
Owner Author

dgrebb commented Oct 23, 2023

@garris it may be time to convert the project to ES Modules. Some of the dependencies have gone all in.

p-map, for example, links to the "Pure ESM package" instructions in their 5.0.0 release notes.

Please let me know your thoughts on this and I will plan out the necessary tasks.

@garris
Copy link
Collaborator

garris commented Oct 24, 2023

Thanks. I don't think this impacts anyone from building or integrating with their environment, right? IOW: It's more for stylistic consistency?

@dgrebb
Copy link
Owner Author

dgrebb commented Oct 24, 2023

This would classify as a breaking change, and probably be a major (7.x.x) release.

package.json would include "type": "module",, and backstop.config.js would be:

export default { Same object as backstop.json }

instead of

module.exports = { Same object as backstop.json }

const ... = require(...) becomes import ... from '...' etc.

Integration with environments may be impacted, especially in cases where users have build BackstopJS directly into CI tooling dashboards or have custom runners written in commonjs.

There's a great article on dev.to which provides the caveats in a ~5m read.

The options are:

  • A) don't upgrade any dependencies higher than their last commonjs versions and continue with non-breaking (but increasingly outdated) standard of commonjs
  • B) upgrade all the things and announce v7 as breaking and dropping support for the commonjs nuances such as require and exports vs. import and export

I don't want to push one way or the other. Long-term benefits, short-term community outrage (but won't there always be? :) vs. short-term updates (limited by any packages that have gone full-module, such as p-map), and less community outrage.

Let me know what you think!

@garris
Copy link
Collaborator

garris commented Oct 25, 2023

I am not against this. How about we do workarounds in the short term and stabilize the project over the next few releases? After getting a few solid releases under our belts (with minimal or no breaking changes) we could plan a more ambitious roadmap with planned breaking changes in the future. How does that sound?

@dgrebb
Copy link
Owner Author

dgrebb commented Oct 25, 2023

@garris sounds good — I've made progress on this and will submit a PR after getting the Dockerfile on Node 20 as well. Created #40 for that.

Thank you for the consideration and feedback on this!

@dgrebb dgrebb removed this from the Node Support (>=18.0.0) milestone Oct 25, 2023
@dgrebb dgrebb self-assigned this Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working chores Maintenance and other feature-unrelated tasks. enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants