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

Add file extension for import statement #30

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

zhiyuanliang-ms
Copy link
Contributor

@zhiyuanliang-ms zhiyuanliang-ms commented Aug 20, 2024

Why this PR?

  1. Follow this PR: [core] Upgrade to ES-Modules for core Azure/azure-sdk-for-js#26238 to add .js extension to import files
  2. Follow this spec: https://nodejs.org/dist/latest/docs/api/esm.html#import-specifiers Notice that 'The file extension is always necessary for these.'

@Eskibear
Copy link
Member

Notice that 'The file extension is always necessary for these.'

This is for ESM. So the first step is to explore pros and cons (esp. potential blockers), and determine whether we want to upgrade to ESM (like azure-sdk core libs). Then this PR can be part of the task.

@zhiyuanliang-ms
Copy link
Contributor Author

zhiyuanliang-ms commented Aug 21, 2024

@Eskibear The question here is to decide between CJS and ESM for the library.

Currently, in the tsconfig file, we configured module as ESNext, which means we have made the choice. The problem here is that we did not strictly adhere to the ESM standards.

The con of ESM is the compatibility issue that some libraries or tools might not fully support ESM, especially if they were designed with CJS in mind. That's the reason why we will build the project in both CJS and ESM styles.

In the readme of this repo, we specified the prerequisites to be Node.js LTS version. ESM has been supported natively in Node.js starting from 14.x. And 14.x has reached the end of its life. https://github.com/nodejs/release#release-schedule
The lifecycle of each Node.js version is relatively short, and the iteration of Node.js is very rapid.

ESM is the standard module system in JavaScript and is the direction in which the whole JS ecosystem is moving. I don't worry the compatibility issue so much.

The JS feature management library will focus on the client side story in the future. ESM is natively supported in modern browsers, which simplifies the setup for front-end projects.

So I think our JS libraries should embrace ESM and encourage our users to do this.

@Eskibear
Copy link
Member

in the tsconfig file, we configured module as ESNext, which means we have made the choice.

"compilerOptions": {
"module": "ESNext",
"outDir": "./dist-esm"
},

It means TS compiler will generate JS code as ESM following ESNext standard, into folder dist-esm, which will be included in our shipped package. See https://www.typescriptlang.org/tsconfig/#module

Indeed the choice has been made since day 1, as we embrace ESM as much as possible.

whole JS ecosystem is moving. I don't worry the compatibility issue so much.

Just note that existing Nodejs tools might handle ESM poorly than they declare. Just to make sure the generated CJS .js code works, in order not to block who are still using CJS (a huge portion).

And one thing you probably need to decide, whether to update file extension from .js to .mjs/.cjs in generated code, esp in dist-esm. As nodejs runtime by default regards .js files as CJS modules if not explicitly specified by parameters. Overall, ESM makes things better, and we should go that direction if possible.

@CharlesLZY
Copy link

Another interesting thing I found is that when I tried to usetshy to build our lbrary, it will report the error:
image
In this PR, sdk team started to use tshyto build their packages. So, adding file extension to all import statement is also required by tshy.

@CharlesLZY
Copy link

CharlesLZY commented Aug 21, 2024

And one thing you probably need to decide, whether to update file extension from .js to .mjs/.cjs in generated code, esp in dist-esm.

@Eskibear
tshy and azure js sdk use .js instead of .mjs for esm build. I will follow them.

Copy link
Member

@Eskibear Eskibear left a comment

Choose a reason for hiding this comment

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

Good to go as it won't block anything.

No idea if tsc has compiler options forcing to check .js extension for relative specifiers. Or if there's linting rules for that. worth a try.

@zhiyuanliang-ms zhiyuanliang-ms merged commit 8711f6a into main Aug 23, 2024
3 checks passed
@zhiyuanliang-ms zhiyuanliang-ms deleted the zhiyuanliang/add-js-extension branch August 23, 2024 15:17
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.

3 participants