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

Refactor client-side utilities to ESM format #50

Open
i-like-robots opened this issue Feb 12, 2019 · 4 comments
Open

Refactor client-side utilities to ESM format #50

i-like-robots opened this issue Feb 12, 2019 · 4 comments

Comments

@i-like-robots
Copy link
Contributor

i-like-robots commented Feb 12, 2019

Currently the client-side JS utilities are written in the CommonJS (CJS) format and export an object containing its API methods, like this:

module.exports = {
  $: () => { ... },
  $$: () => { ... }
};

In many cases the applications and components using these utilities are doing so by importing them using the ECMAScript module (ESM) format, but these are not compatible and there is ambiguity about how to handle this behaviour. We currently rely on our tools allowing this ambiguity in order to compile our code.

// What does this mean!?
import { $ } from 'n-ui-foundations'

The tools and workarounds which have enabled us to loosely mix-and-match module formats can no longer be used so we now need to fix problems like this.

ESM has no equivalent to module.exports = {}. With ESM export default is really just another named export, more closely equivalent to exports['default'] in CJS.

The most straightforward solution therefore is to refactor this module to use named exports, in either CommonJS or ESM format. However, this would break any existing CJS code.

There are a variety of uses across our codebase:

https://github.com/search?l=JavaScript&q=org%3AFinancial-Times+n-ui-foundations&type=Code

@i-like-robots
Copy link
Contributor Author

i-like-robots commented Feb 12, 2019

Example usage in an ESM file (import default export):
None found!

Example usage in an ESM file (import named export):
https://github.com/Financial-Times/next-profile/blob/master/client/main.js

Example usage in a CJS file (require default):
https://github.com/Financial-Times/next-corp-signup/blob/master/client/routes/signup-form-b2b/actions/on-typing-a-job-title.js

Example usage in a CJS file (require named property):
https://github.com/Financial-Times/n-myft-ui/blob/master/myft/ui/personalise-links.js

@chee
Copy link
Member

chee commented Feb 12, 2019

as a transitional, what about exporting the named exports indiviually, and also exporting the named exports on default.
that seems like the least code changes and doesn't even have to be a major version (the code changes happen at the library level, not at app level)

export {a, b, c}
export default {a, b, c}

@leggsimon
Copy link
Contributor

I think @chee’s proposal is the best, we export as both and release as a minor to give us time to transition. I wonder whether we could even put some warning in the build/install step to ask people that this will be deprecated in the next major version and that we should start attempting to use the named exports now.

The only other option I see is a breaking change and major release, which we could do:

A) while publishing minors and patches to v3.x and the new v4.x, which seems like more work.
B) while only adding additional functionality to v4.x thus forcing anyone to upgrade if they want new stuff.

If we did a major and B, we could do what we did before and sit everyone down and do all the apps together. This might be a good thing because modules in all it’s incarnations is wild and confusing and so it might be good for learning.

@i-like-robots
Copy link
Contributor Author

I have realised that my statement about a change to named exports breaking commonJS code expecting a default export was incorrect.

If we refactor this module to use named CommonJS exports it should not break any existing code, and this would continue to work (as I incorrectly stated in the issue previously 😶 ):

const foundations = require('n-ui-foundations')

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

No branches or pull requests

3 participants