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

Refactored module exports; #5162

Merged
merged 5 commits into from Oct 30, 2022
Merged

Conversation

DigitalBrainJS
Copy link
Collaborator

@DigitalBrainJS DigitalBrainJS commented Oct 19, 2022

Brief description:

  • require can only be used with default export as it was in 0.x versions;
  • import can be used with both default & named exports;

Detailed:

  • Added browser CommonJS bundle to the build pipeline;
  • Fixed exports entry points;
  • Added module exports tests;
  • Added missing ESM export for parity with Axios factory;
  • Added toFormData, formToJSON, isAxiosError, spread, isCancel, all as named export to index.d.ts;

In an attempt to bring order, the package export has been refactored. The following rules were adopted:

  • CommonJS module has only factory as default export, as it was before
  • CommonJS entry point to build the cjs bundle is ./lib/axios.js
  • ESM entry provides both default (factory) and named exports;
  • ESM entry point is ./index.js. This module should expand Axios factory export as ESM named export. This module should not be used for purposes other than re-export features as a named export.
  • Named export should be in parity with export provided by Axios factory
  • The ./dist/axios.js and ./dist/esm/axios.js bundles are for CDN usage only.

Closes #5154; Closes #5164; Closes #5169; Closes #5091; Closes #5130; Closes #5118; Closes #5011; Closes #5031; Closes #5044; Closes #5082;
Related: #5101, #5026, #5088

Users that import the module with require will do it as it was before:

const axios = require('axios');

console.log(axios.isCancel('something'));
console.log(new axios.AxiosError('oops'));

If the module is loaded with import statement you can use both named and factory exports:

import axios from 'axios';

console.log(axios.isCancel('something'));
console.log(new axios.AxiosError('oops'));

OR

import axios, {isCancel, AxiosError} from 'axios';

console.log(isCancel('something'));
console.log(new AxiosError('oops'));

If something goes wrong (custom/outdated environment), you can try to load CommonJS bundle directly:

const axios = require('axios/dist/browser/axios.cjs'); // browser
// const axios = require('axios/dist/node/axios.cjs'); // node

console.log(axios.isCancel('something'));
console.log(new axios.AxiosError('oops'));

Added module exports tests;
Added missing ESM export for parity with Axios factory;
Added `toFormData`, `formToJSON`, `isAxiosError`, `spread`, `isCancel`, `all` as named export to `index.d.ts`;
test/module/test.js Fixed Show fixed Hide fixed
Promise.resolve(2)
];

const promise: Promise<number[]> = all(promises);

Check notice

Code scanning / CodeQL

Unused variable, import, function or class

Unused variable promise.
// axios.spread named export
(() => {
const fn1 = (a: number, b: number, c: number) => `${a}-${b}-${c}`;
const fn2: (arr: number[]) => string = spread(fn1);

Check notice

Code scanning / CodeQL

Unused variable, import, function or class

Unused variable fn2.
// named export

if (isAxiosError(error)) {
const axiosError: AxiosError = error;

Check notice

Code scanning / CodeQL

Unused variable, import, function or class

Unused variable axiosError.
it('should be able to be loaded with require', async function () {
this.timeout(30000);

await exec(`npm test --prefix ${pkgPath}`);

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1).
Added missed `CanceledError` & `AxiosHeaders` to `AxiosStatic` interface;
it('should be able to be loaded with import', async function () {
this.timeout(30000);

await exec(`npm test --prefix ${pkgPath}`);

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1).
it('should be able to be loaded with import', async function () {
this.timeout(30000);

await exec(`npm test --prefix ${pkgPath}`, {});

Check warning

Code scanning / CodeQL

Shell command built from environment values

This shell command depends on an uncontrolled [absolute path](1).
@KrayzeeKev
Copy link

What is the process for these getting released? Is there a regular release cadence? Or is it waiting for some magic? Or does it happen manually when there's enough to warrant a release?

@DigitalBrainJS
Copy link
Collaborator Author

DigitalBrainJS commented Nov 2, 2022

@jasonsaayman Can we have a pre-release with this PR like 1.2.0-beta.1? It would be nice to get early user feedback on this. In case when exports are broken a separate release/pre-release is desirable to avoid implicit collisions with other PRs that change the export in some way.

@jasonsaayman
Copy link
Member

@KrayzeeKev no there is no set schedule. I would like to implement something like that but it is pretty hard as I also have work to attend too. I will look into doing something like a monthly schedule. @DigitalBrainJS yeah sure, the only problem is it will get very little traction, for the entire time we pre-released v1 we got 3000 downloads, pretty sure about 200 were my own projects.

@jasonsaayman
Copy link
Member

But yeah I will cut it as mentioned this weekend.

@KrayzeeKev
Copy link

@jasonsaayman Thx. I'm with you on the whole pre-release thing. We can't use them. It's npm update or nothing for us. I'd happily try one on my local for you but the fundamental problem we have here is we're running 0.27 because nothing above 1.0 works (axios-retry and nock in our ecosystem) And my security people are looking at that ^0 and not liking me :-) I might see if I can contort firewalls and proxies sufficiently to test the latest github branch on this but I've failed to achieve that in the past. The world really wants me to use npm versions (internal npm cache, no direct internet, etc, etc)

@KrayzeeKev
Copy link

I've managed to pull this version in using https but our code has always used: require('axios').default;
I don't know why. It was like that when I got it. Now it breaks. Is that the result of this PR or some other 1.x change? Does require('axios') give us now exactly what .default gave us before?

@DigitalBrainJS
Copy link
Collaborator Author

@KrayzeeKev They are equivalent:

module.exports = axios;
module.exports.default = axios;

Just re-added support for this for backward compatibility in #5225 .

@KrayzeeKev
Copy link

@DigitalBrainJS Thanks. Easy to fix on our end too. Now I have to go back through the tickets that this merge is supposed to fix so I can tell everybody which ones it doesn't (axios-retry still broken with a function body in an http header)

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