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

Convert webflow example to use esnext modules #15940

Merged
merged 8 commits into from
Jun 16, 2023

Conversation

Abe27342
Copy link
Contributor

@Abe27342 Abe27342 commented Jun 9, 2023

Description

This makes progress toward #15917 by converting @fluid-example/webflow to use esnext modules.

This is the most involved update of any of the packages that depend on @fluid-internal/test-version-utils, since ESNext modules don't support the require.extensions api (they don't support require), which was the technique used to make webpack-style CSS imports not blow up when run in a node environment. Instead, it's recommended to use the node esm loader API. Although this API is experimental, it hasn't receive any changes between v16.12 and the node latest (v20.x).

@Abe27342 Abe27342 requested review from msfluid-bot and a team as code owners June 9, 2023 21:32
@github-actions github-actions bot added area: examples Changes that focus on our examples dependencies Pull requests that update a dependency file base: main PRs targeted against main branch labels Jun 9, 2023
@Abe27342
Copy link
Contributor Author

esm-loader-css has very few projects depending on it, but I looked through their code and it looks good. An alternative would be to define our own bare-bones loader:

const extensionsRegex = /\.css$/;

/**
 * This uses the node loader API (see https://nodejs.org/docs/latest-v16.x/api/esm.html#loaders)
 * to enable webpack-style imports of .css modules.
 *
 * This is the recommended successor to `require.extensions`, which was used by https://www.npmjs.com/package/ignore-styles
 * to accomplish the same thing.
 */
export async function load(url, context, nextLoad) {
	if (extensionsRegex.test(url)) {
		return {
			format: "json",
			shortCircuit: true,
			source: "{}",
		};
	}

	// Defer to the next hook in the chain.
	return nextLoad(url, context);
}

but this seems like it'd have more maintenance burden and doesn't handle edge cases as well (e.g. the fact that the loader api changed in 16.12).

@Abe27342 Abe27342 requested a review from a team as a code owner June 12, 2023 19:24
@github-actions github-actions bot added the area: tests Tests to add, test infrastructure improvements, etc label Jun 12, 2023
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Jun 13, 2023

@fluid-example/bundle-size-tests: +529 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 448.33 KB 448.34 KB +6 Bytes
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 239.04 KB 239.04 KB +2 Bytes
loader.js 155.54 KB 155.55 KB +4 Bytes
map.js 46.75 KB 46.75 KB +2 Bytes
matrix.js 146.68 KB 146.68 KB +2 Bytes
odspDriver.js 92.46 KB 92.47 KB +6 Bytes
odspPrefetchSnapshot.js 43.69 KB 43.69 KB +4 Bytes
sharedString.js 163.28 KB 163.29 KB +2 Bytes
sharedTree2.js 248.25 KB 248.74 KB +499 Bytes
Total Size 1.7 MB 1.7 MB +529 Bytes

Baseline commit: fd14d50

Generated by 🚫 dangerJS against f2f9b26

Copy link
Contributor

@justus-camp justus-camp left a comment

Choose a reason for hiding this comment

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

I was going to leave a comment about using --experimental-loader, but it seems like you did your due-diligence there. I don't know how much we gain from using esm-loader-css. If the API changes and the package is unmaintained, don't we end up in the same situation as using our own implementation?

Comment on lines +9 to +12
export { FlowDocument } from "./document/index.js";
export { Editor } from "./editor/index.js";
import { WebFlow, WebflowView } from "./host/index.js";
export { htmlFormatter } from "./html/formatters.js";
Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly for my own understanding, but does anything currently enforce the pattern of creating an index.ts file and re-exporting from there? If so, does the change to ESNext have any effect on that enforcement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. My understanding of the more recent history of it is that:

index.js files are handled specially by the CJS module loader in that an import to ./foo would attempt to look for files ./foo.js, ./foo/index.js, and analogous files with any extensions registered by require.extensions.
I'm guessing this behavior came out of the fact that JS package publishers did not have good ways to "map" the files in their project to valid imports for consumers, so an index convention was settled.

Since then, the esnext format has largely solved that problem (packages can expose subpaths, import specifications are much more well-behaved and understood across the ecosystem) and so does away with the more egregious artifacts of cjs loaders.

All these changes do is basically make "where is this file located" more explicit. You'll still get the same compile-time validation if you try to import from ./has-no-index/index.js but that folder is bogus.

examples/data-objects/webflow/package.json Show resolved Hide resolved
@Abe27342
Copy link
Contributor Author

I was going to leave a comment about using --experimental-loader, but it seems like you did your due-diligence there. I don't know how much we gain from using esm-loader-css. If the API changes and the package is unmaintained, don't we end up in the same situation as using our own implementation?

Perhaps, but the package already does better than writing our own loader in 2 cases:

  1. It works on node before 16.12 (we technically don't support this by our "engines" config, but I think most things in our repo work for any node 16, and I don't see a real reason to explicitly break that)
  2. It's less code for us to maintain, and the approach scales better if we have other web examples we convert to esnext with .css imports and mocha tests--we'd otherwise need to move our own custom loader to some place we can reference it. Using esm-loader-css is just a matter of adding a devDep and some NODE_OPTIONS flags.

@Abe27342 Abe27342 merged commit dac7f34 into microsoft:main Jun 16, 2023
@Abe27342 Abe27342 deleted the webflow-esm branch June 16, 2023 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: examples Changes that focus on our examples area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants