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

fix: Revert type=module change #357

Merged
merged 2 commits into from
Jun 21, 2023
Merged

fix: Revert type=module change #357

merged 2 commits into from
Jun 21, 2023

Conversation

timphillips
Copy link
Contributor

The changes in #341 introduced an issue with importing RavelinJS via the npm package (reported in this internal Slack thread).

The default import (e.g. import Ravelin from 'ravelinjs/core') now returns undefined instead of the expected Ravelin constructor function. Ravelin is instead being initialised as a global variable.

The format of the bundles published in v1.8.0 are identical to v1.7.1. It appears that the "type": "module" change in package.json impacts how the package is imported in build systems. This reverts the change until we determine the magical combination of config options required for a proper fix.

#344 was also reverted because it depends on #341 (though I've preserved the change to target IE11 in tests).

This reverts commit f431aed.

# Conflicts:
#	package.json
This reverts commit dff1e65.

# Conflicts:
#	package.json
Copy link

@ericmatteucci ericmatteucci left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for this

Copy link
Contributor

@talon266 talon266 left a comment

Choose a reason for hiding this comment

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

LGTM
I think we need to determine what's a module and what's commonjs and perhaps split it into 2 projects?
https://nodejs.org/api/packages.html#type

@timphillips timphillips merged commit 4883a24 into v1 Jun 21, 2023
@timphillips timphillips deleted the revert-type-module branch June 21, 2023 14:09
@icio
Copy link
Contributor

icio commented Jul 10, 2023

Bah, apologies for this. I think the part I forgot to update was:

file: 'dist/' + basename(bundle),
format: 'umd',
name: 'Ravelin',
esModule: false,
exports: 'default',

This should probably be:

   file: 'dist/' + basename(bundle),
-  format: 'umd',
-  name: 'Ravelin',
-  esModule: false,
-  exports: 'default',
+  format: 'es',

However, it's potentially going to be a nuisance getting require and import to work in a backwards-compatible manner when switching to modules. require isn't technically supported by ESModules. The switch to ESModules could just be part of the v2 branch in #345 so that we don't need to worry about supporting both styles.

@icio
Copy link
Contributor

icio commented Jul 13, 2023

Some good links here on the matter: https://deno.com/blog/commonjs-is-hurting-javascript.

Anyway, peace ✌️ Hope to see you all soon.

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