Skip to content
This repository has been archived by the owner on Jul 15, 2021. It is now read-only.

Add support for ECMAModules import #112

Merged
merged 6 commits into from
Jan 11, 2021
Merged

Conversation

budnix
Copy link
Member

@budnix budnix commented Dec 14, 2020

The PR improves how the distributed files are published to the NPM. From now on, the module should support not only Node require syntax but import syntax as well. The changes forced a dependency upgrade.

The dist files are tested using different bundlers and environments. The following bundlers are tested:

  • Rollup 1 and 2;
  • Webpack 2, 3, 4, and 5;
  • Parcel;
  • NodeJS 6 and 15 using native require and import syntax.
  • The UMD files (including minified) are tested on the most popular browser. I've tested on IE9-11 and haven't seen any problems.

The commit includes following changes:
 * Upgrade dependencies;
 * Upgrade the process of building the dist files. The package from now
   on should be ready for ECMAModules spec.
The label declaration syntax causes transpilation errors. As it is not
used by the parser it is removed.
@budnix budnix marked this pull request as ready for review December 16, 2020 08:35
Copy link
Contributor

@mrpiotr-dev mrpiotr-dev left a comment

Choose a reason for hiding this comment

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

Seems to work as expected. By the way, I've noticed a few tiny things:

  1. lockfile version is bumped to V2 - maybe we should add npm's min version in engines entry in package.json?
npm WARN read-shrinkwrap This version of npm is compatible with lockfileVersion@1, but package-lock.json was generated for lockfileVersion@2.
  1. On Win10 I had a problem with EOL - I guess .gitattributes file should solve the problem.

  2. On Win10/macOS10.15, npm 7.3.0, node15.4.0 I got 4 failure in tests - all of them are related to floated numbers precision (I guess it's @handsontable/formulajs issue)

  3. Should we adjust @handsontable/formulajs package? At the moment we import dynamically required files from @handsontable/formulajs/index.js (it's a main entry point for that package). It is the "dual package hazard" we try to avoid, isn't it?

But I think the above list is not required to finish this PR - it's up to you 🙂
Well done @budnix!

@mrpiotr-dev mrpiotr-dev assigned budnix and unassigned mrpiotr-dev Dec 22, 2020
In includes:
 * Regenerate the package-lock.json file compatible with Node v10;
 * Add .gitattributes file;
@budnix
Copy link
Member Author

budnix commented Jan 4, 2021

Thanks for the review :)

  1. lockfile version is bumped to V2 - maybe we should add npm's min version in engines entry in package.json?
npm WARN read-shrinkwrap This version of npm is compatible with lockfileVersion@1, but package-lock.json was generated for lockfileVersion@2.

I recreated the file (771b36b) to be compatible with the NodeJS version used within the project (v10).

  1. On Win10 I had a problem with EOL - I guess .gitattributes file should solve the problem.

I've added the file in this commit 771b36b.

  1. On Win10/macOS10.15, npm 7.3.0, node15.4.0 I got 4 failure in tests - all of them are related to floated numbers precision (I guess it's @handsontable/formulajs issue)

Yes, depends on used the NodeJS version the precision differs. So far I have added to the contributing.md file a point that describes the NodeJS version requirement.

  1. Should we adjust @handsontable/formulajs package? At the moment we import dynamically required files from @handsontable/formulajs/index.js (it's a main entry point for that package). It is the "dual package hazard" we try to avoid, isn't it?

The @handsontable/formulajs package offers only the CommonJS syntax so I don't think that there is a possibility for "dual package hazard".

@mrpiotr-dev mrpiotr-dev self-requested a review January 4, 2021 15:46
Copy link
Contributor

@mrpiotr-dev mrpiotr-dev left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants