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

Distribute ES module #875

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Silic0nS0ldier
Copy link

@Silic0nS0ldier Silic0nS0ldier commented Jun 9, 2021

Intent in this PR is to convert the existing source into a valid ES module which can be distributed. More could be done, but IMO would be too much all at once.

Maintaining backwards compatibility where possible was a goal with these changes, however there is a limit to how far this can go. Even if the existing files (papaparse.js and papaparse.min.js) were not modified, the presence of the ES module (and exports in package.json) are highly likely to impact downstream projects in the form of build breaks and runtime errors. These are issues which are easier to overcome when not compounded by other significant changes, which is where backward compatibility efforts were spent.

For consumers, changes of note are;

  • New exports (may break tooling that looks up package contents using require.resolve or import.meta.resolve that is not defined in the export map)
    ...
    "exports": {
    	"import": "./papaparse.mjs",
    	"require": "./papaparse.js"
    },
    ...
  • Worker support requirements have changed.
  • Tested NodeJS versions have changed to reflect new releases and old releases leaving their respective support periods.

For maintainers, changes of note are;

  • The base source is now papaparse.mjs, formerly papaparse.js
  • Grunt has been replaced by RollupJS, which handles generation of UMD modules papaparse.js and papaparse.min.js.
  • ESLint updated from ^4.19.1 to ^7.28.0
  • ESLint config tweaked to parse source as ES modules with ECMA version 2020 (minimum for import.meta support)
  • Workers now depend on import.meta.url for ES modules and document.currentScript.src for UMD in browser, and identify themselves as PapaParse created workers via the worker scope name (formerly used presence of blob in script URL).
  • Mocha and mocha-headless-chrome updated.
    Primary driver was to address crashes in an old version of puppeteer.
    One change of note is that Mocha (now at ^9.0.0) supports loading ES module test files (not currently used).

Related

#748
#813

- Use RollupJS and Terser for bundling
- Bumped up dev dependency versions
- Bumped up Travis NodeJS version matrix
- Refactored Papa worker identification
- Refactored worker logic to use `import.meta.url` or `document.currentScript.src`
mholt#748
mholt#813
@Silic0nS0ldier Silic0nS0ldier marked this pull request as ready for review June 9, 2021 05:56
@Silic0nS0ldier
Copy link
Author

@pokoli
Copy link
Collaborator

pokoli commented Jun 9, 2021

Hi Jordan,

Thanks for your work on the topic, this is a step forward on the library.
I did not have a look at the code but from your comments I think this requires a major release as rewrites most of the library code.

Did you have some remarks for me to take in consideration before testing it? Should we update something of the documentation/readme?

@pokoli
Copy link
Collaborator

pokoli commented Jun 9, 2021

@Silic0nS0ldier Test are failing for Node10 Could you please check it?
Feel free to remove support of Node10 if this eases the adoption of this feature.

@Silic0nS0ldier
Copy link
Author

Its bark is worse then its bite here. API surface remains unchanged, most of the noise comes from the removal of an indent layer across the bulk of the main source file and GitHub struggling to understand that a rename has occurred (usually happens when generated files are tracked). Using a separate diff tool and temporarily adding back the indent layer should help to put focus in the right places.

Definitely needs a major release here. The worker logic change represents a breaking change (changed requirements) and the addition of the es module will almost certainly break someone's build (from experience).

Any documentation changes would be to cover changes worker requirements and including the es module in docs as appropriate (there are a few links to the umd and mini umd scripts).

@Silic0nS0ldier
Copy link
Author

CI test failure comes down to Mocha v9 dropping support for NodeJS 10. Dropping down to v8 should address that.

@pokoli
Copy link
Collaborator

pokoli commented Jun 9, 2021

@Silic0nS0ldier I see node10 is no longer supported so I removed support for it in #876

Note that we are no longer using the Travis CI infrastructure so I removed the file to avoid confusions.

Could you please rebase the PR to latest master branch? This will only run the tests with supported node versions, so you should be able to bump the mocha version back to 9.0

Sorry for the confusions.

@Silic0nS0ldier
Copy link
Author

I did an integration test with an existing project. Only issue was a minor tooling break due to require.resolve behaviour changes when the exports field is used, an easy to fix issue that would have come up in time.

@pokoli
Copy link
Collaborator

pokoli commented Jun 10, 2021

@Silic0nS0ldier Good news. Could you ping me back when you've finished the integration project so I can come back to test it here?

Also I'm wondering why you used RollupJS? Correct me if I'm wrong but is not also possible to use webpack? Which are the benefits of using RollupJS?

@Silic0nS0ldier
Copy link
Author

Could you ping me back when you've finished the integration project so I can come back to test it here?

Project in question is closed source sorry.

Also I'm wondering why you used RollupJS? Correct me if I'm wrong but is not also possible to use webpack? Which are the benefits of using RollupJS?

I mostly opted for RollupJS as its what I have experience using.

Beyond that though, the consensus I've seen is that webpack is best suited to apps while RollupJS is best suited to libraries (no real justification for why that is though). Key difference I see is that historically RollupJS produced a more optimised output, I understand that as of webpack v2 the difference became much less significant.

As for benefits;

  • Smaller API surface means less breaks when updating (and simpler configurations).
  • More narrow scope means less reason for breaking changes to occur.
  • In my experience its adhered more closely to the ES module spec (this should only improve now that NodeJS has unflagged ESM support).

There are quite a few other alternatives out there now (most targeting webpack). Some of them are;

  • esbuild
  • @web/dev-server (alternative to webpack dev server)
  • snowpack
  • parcel
  • microbundle (uses rollup)

@jimmywarting
Copy link
Contributor

jimmywarting commented Jun 27, 2021

I would say drop support for commonjs entierly and do a major release (there are other popular modules that have already done so)

@Silic0nS0ldier
Copy link
Author

@jimmywarting Current versions of PapaParse are distributed as a UMD (specifically the pattern https://github.com/umdjs/umd/blob/master/templates/returnExports.js) which supports binding to window, CommonJS export, (I think) AMD, and JQuery. If support for older integration approaches is to be dropped, might as well drop them all.

I'll await for a consensus to form before making any changes on this front.

@pokoli
Copy link
Collaborator

pokoli commented Jun 28, 2021

I do not think we should drop support fort commonjs.
IIUC we can support it without too much effort and I do not see any improvement by droping it's support. @jimmywarting please give some rational to your proposal if you don't agree.

@jimmywarting
Copy link
Contributor

jimmywarting commented Jun 28, 2021

IIUC we can support it without too much effort and I do not see any improvement by dropping it's support.

That's true, You will not benefit so much of it right now if you can output both UMD and ESM.

please give some rational to your proposal if you don't agree.

  • Less code (duplicate files)
  • Open up to import other modules built with only ESM, (commonjs need to use the dynamic import() b/c ESM are async)
  • Support for top level await (could do conditional loading of polyfills)
  • basically all maintained version of node and evergreen browser support ESM right now. IE is pretty much dead now.
  • pushes the hole js community forward towards newer standardized way of doing things

ref: https://redfin.engineering/node-modules-at-war-why-commonjs-and-es-modules-cant-get-along-9617135eeca1

node-fetch, fetch-blob, formdata-polyfill are 3 that i know of that only support ESM and have ditched commonjs, or any UMD pattern

@Silic0nS0ldier
Copy link
Author

Regarding shipping ESM only, there is one major consideration. Should a project be consuming the CJS and ESM versions of the source (this is possible, particularly if PapaParse is used in another library and directly), global configuration won't be shared. While it is possible to address this by having global configuration stored in a common CJS file, doing so is likely to change the API surface.

Shipping ESM only can however present challenges on the consumer side where ESM is not fully adopted. Tools like webpack are quite good at hiding missing ESM support and introducing behaviour which does not exist in native ESM. This is a problem for a monolithic project I work on which uses PapaParse in the following contexts;

  • Unit tests with Jest, ESM source is converted to CJS. Crashes if PapaParse is ESM only.
  • Running locally (fake mode), ESM source is converted into CJS (with lots of webpack glue) that has ESM interops. Works.
  • Production, same as running locally but heavily optimised (though webpack glue remains).

Moving to proper ESMis something being worked towards, however there are a lot of hurdles to overcome. As such I'm in favour of keeping a CJS export around for at least the next release.

@architucas
Copy link

any update on ESM release?

@Silic0nS0ldier
Copy link
Author

Not sure if I'm the holdup here, but I won't have time to make any changes to this PR. If others are waiting on this, feel free to open another PR (link to it, and I'll close this PR)

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.

None yet

4 participants