[Feature]: Add `.js` file extension to built output to improve native ESM support

spmonahan commented 6 months ago

Library

React Components / v9 (@fluentui/react-components)

Describe the feature that you would like added

I want Fluent's ESM output (CJS can go either way I think) to include the .js extension for all relative JS imports. For example,

// Source Typescript
import { feature } from './file';

// Output ESM Javascript
import { feature } from './file.js';

This update will better allow Fluent's ESM output to be loaded directly in the browser (import maps will still be required to resolve bare package imports but that's no problem).

Ideally we should do this without changing source code and leverage something like SWC's @swc/plugin-transform-imports (which transforms exports in addition to imports despite the name).

Have you discussed this feature with our team

@hotell @chrisdholt

Additional context

Currently the ESM output from Fluent is "extensionless" which prevents the ESM output from working correctly in a browser.

// Example output of current output
import { FluentProvider } from '@fluentui/react-components'; // bare module
export { something } from './somewhere'; // bare JS file

// Example of what this needs to be for native ESM support
import { FluentProvider { from '@fluentui/react-components;
export { something } from './somewhere.js';

In this example there is a "bare module" import where code is imported from a package name (that lacks a .js extension) and a "bare JS file" import. The latter is typically used within a package to import other JS files using relative paths.

We can address "bare modules" with importmaps, allowing import $whatever from 'some-module-without-an-extension' to resolve correctly but importmaps cannot, effectively^†, handle the internal bare JS imports. To handle the bare JS imports we must update the output to include the .js extension.

^† I say effectively because it is possible to generate an importmap for all the base JS imports within a package but such an importmap would be quite large and then require end users to correctly merge it with any other importmaps they are using. Technically possible but not a good solution :)

Related issues

23508

Validations

[X] Check that there isn't already an issue that request the same feature to avoid creating a duplicate.

Priority

Normal

Hotell commented 6 months ago

@miroslavstastny I'm re-assigning this to v-build as we will be shipping it

Hotell commented 6 months ago

Update:

Previously I mentioned that in order to make this work, our packages need to be either type:module or ship .mjs under exports#import mapping.

I explored this further and possiblity to add "just .js extensions" is also possible but has some issues which I'll expand on in 1st option description.

Approach options:

1. adding explicit .js extension on build time

NodeJS: we use exports#node mapping within our packages so no matter if we ship native ESM, if our packages are loaded in node, it will always consume commonjs outputs ATM
Browser: if one want's use native ESM, package modules need to be compliant with ESM format - all imports/exports need to specify extension. Export maps won't work with raw native ESM, so if a package includes re-exports from another package via bare import/export specifier (export {Hello} from '@fluentui/hello';) user is forced to provide import-maps, otherwise it will throw error

👁️ This has following gotchas:

We need to enforce that relative imports always point to existing physical file no matter what module loader is in place (node vs browser).

🧪 Example:

// @filename src/index.ts
export {Hi} from './hello'

// @filename src/hello/index.ts
export {Hi} from './hi'

⬇️ swc transpile ( + @swc/plugin-transform-imports ) ⬇️

// @filename lib/index.js
// 🚨🚨🚨 this is invalid path - will throw in all environements
export {Hi} from './hello.js'

// @filename src/hello/index.js
export {Hi} from './hi.js'

Rollup handles this as expected

// @filename src/index.ts
export {Hi} from './hello'

// @filename src/hello/index.ts
export {Hi} from './hi'

⬇️ rollup ⬇️

// @filename lib/index.js
// ✅ ✅ ✅ unwraps the import path to it's source 
export {Hi} from './hello/hi.js'

// @filename src/hello/index.js - wont be created
// ✅ ✅ ✅ nested barrel files are completely removed from output as their are not necessary

What are our implementation options:

switching to rollup
swc adds implementation to properly transpile module import/export specifiers like rollup
- module directories can be unwrap via module.resolveFully, with that adding extensions via import plugin wont work
- https://github.com/swc-project/swc/issues/8742
implement lint rule that will verify user always specifies import/export from existing file name ( we have something like this for old AMD code https://github.com/microsoft/fluentui/blob/master/scripts/tasks/src/lint-imports.ts)

2. declaring package as ESM native via type:module pragma

use .js extension within imports within package scope + everything mentioned in 1) applies

3. shipping all ESM files with .mjs extension

By default in ESM native environments, if a package.json has no type specified it fallbacks to type:commonjs by default (our packages situation).

While we have export maps in place, they are invalid for native ESM loader ( it works solely for build tools with current output ).

In order to support native ESM loader without making the package native ESM, all files that are accessed via exports#import need to:

have .mjs extension
use .js extension within imports within package scope

Implementation requirements:

NOTE: solutions need changes to webpack ( because storybook, cypress use webpack v4)

1. adding explicit .js extension on build time

build time only (won't work reliably with current setup, see 1st Approach options: above )

2. declaring package as ESM native via type:module pragma

tooling changes
- just-script patch needed
- jest moduleMapper hacks / '^(\\.{1,2}/.*)\\.js$': '$1',
source changes
- TypeScript: moduleResolution": "NodeNext" / modifying all imports in .ts files to include .js extension

3. shipping all ESM files with .mjs extension

build time only
- ESM transpile output has .mjs extension and explicit .mjs within import statement
- opt-in build mechanism needed to be able to have this only on packages we intent to
- 🚨 doesn't work with griffel https://github.com/microsoft/griffel/issues/513
- 🚨 source maps will probably not work as expected

spmonahan commented 6 months ago

Re-opening since we had to revert this in #30803

yuntian001 commented 5 months ago

I need it. When do you plan to fix it

AnujAgrawal-1 commented 2 months ago

@spmonahan @Hotell Any update on above issue as i am also getting same issue SyntaxError: Unexpected token 'export' at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1728:14) at Object.<anonymous> (node_modules/@fluentui/react-provider/lib-commonjs/components/FluentProvider/renderFluentProvider.js:15:20)

microsoft / fluentui