whitlockjc / json-refs

Various utilities for JSON Pointers (http://tools.ietf.org/html/rfc6901) and JSON References (http://tools.ietf.org/html/draft-pbryan-zyp-json-ref-03).
MIT License
223 stars 63 forks source link

ESM #162

Closed brettz9 closed 1 year ago

brettz9 commented 5 years ago

I didn't integrate the Rollup process into Gulp; though there is a programmatic API for Rollup you can use, I get the sense projects are moving away from Gulp/Grunt in favor of just npm command line scripts so thought it may be best to go that route.

Besides integration into (or replacement of) Gulp (and for that matter, anything that may be needed for Karma), I haven't currently done the work of adding a browser-specific build which either uses a plugin like rollup-plugin-re to strip out (or no-op) import statements not relevant for the browser or which embeds polyfills for the Node builtins (there is a plugin for this too). It would be helpful if you might be able to summarize which of these: need polyfills and which aren't used in the browser: path, querystring, fs, tty, util, crypto, events, buffer, string_decoder, stream, os, http, https, url, zlib

There are also more refactoring changes I think could be done, such as preferring const/let over var, using async/await over Promises in source, etc., including for test files and the README, but I'd recommend first also adding a more robust ESLint config. There are projects like "standard", "airbnb", and I naturally prefer my own "eslint-config-ash-nazg" which I think catches many issues that the others don't (it is mostly an integration of other existing configs and rules). My project also includes eslint-plugin-jsdoc which can replace the now deprecated valid-jsdoc rule you had, as well as eslint-plugin-markdown for checking JavaScript in your Markdown doc files.

As one FYI, if your dependencies ever exported themselves as ESM modules, you could avoid the need for rollup-plugin-commonjs in Rollup.

brettz9 commented 5 years ago

In some cases, e.g., I'd guess "querystring", it may be better to prefer the browser variant URLSearchParams and just polyfill for any older versions of Node (assuming you still wish to support them), allowing you to drop the polyfill in the future and avoid Node/browser-specific code for such APIs.