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

Re-added old bundles #439

Merged
merged 1 commit into from
Oct 6, 2023
Merged

Re-added old bundles #439

merged 1 commit into from
Oct 6, 2023

Conversation

btaillon-coveo
Copy link
Contributor

@btaillon-coveo btaillon-coveo commented Oct 6, 2023

Context

Before the breaking change

Prior to #433, it was not fully possible to import directly from coveo.analytics like so:

import {CoveoAnalyticsClient} from 'coveo.analytics';

This was because the package.json file contained these lines:

{
    "main": "dist/library.js",
    "module": "dist/library.es.js",
    "browser": "dist/coveoua.browser.js",
}

While library.js and library.es.js contained modules, coveoua.browser.js did not (it globally defines "coveoua"), and the "browser" entry caused some bundlers to try and fail to use it. Additionally, library.es.js was using expecting browser APIs while library.js was expecting Node.js APIs.

The breaking change

#433 introduced a way to import from coveo.analytics like so:

import {CoveoAnalyticsClient} from 'coveo.analytics/modules';

In that process, to avoid confusion between Node CommonJS, Node ESM, browser ESM, and browser IIFE, I:

  • renamed library.js to library.cjs
  • added library.esm
  • renamed library.es.js to browser.mjs

Unfortunately, some of our own implementations (and likely some external) were using a workaround which involved forcefully telling their bundlers to use library.es.js. Since this file was renamed, those now crash.

Solution

I added copies of the renamed files to their former names.

Specifically:

  • browser.mjs gets copied to library.es.js
  • library.cjs gets copied to library.js

Copy link

@alexprudhomme alexprudhomme left a comment

Choose a reason for hiding this comment

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

Nice solution !

@btaillon-coveo
Copy link
Contributor Author

confirmed that this works by testing with npm link and a separate repo.

@btaillon-coveo btaillon-coveo merged commit 63a226b into master Oct 6, 2023
3 checks passed
@btaillon-coveo btaillon-coveo deleted the KIT-2808 branch October 6, 2023 16:54
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