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

Enhance build process for ES module support and update Sinon imports #7589

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

anmolshres98
Copy link
Contributor

@anmolshres98 anmolshres98 commented Jan 22, 2025

Make core-backend support esm build output as part of wider 5.0 effort

Copy link
Member

@aruniverse aruniverse left a comment

Choose a reason for hiding this comment

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

is this really all it took to convert core-backend to run in esm?
how did you test this?

please review changes from #7293

@@ -18,7 +18,7 @@ import {
import { ElementOwnsChildElements, ElementOwnsUniqueAspect, SubjectOwnsPartitionElements } from "../../NavigationRelationship";
import { IModelTestUtils } from "../IModelTestUtils";
import { KnownTestLocations } from "../KnownTestLocations";
import Sinon = require("sinon"); // eslint-disable-line @typescript-eslint/no-require-imports
import * as Sinon from "sinon";
Copy link
Member

Choose a reason for hiding this comment

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

theres already imports from sinon in these files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@anmolshres98
Copy link
Contributor Author

anmolshres98 commented Jan 22, 2025

is this really all it took to convert core-backend to run in esm? how did you test this?

please review changes from #7293

In the fundamental sense, yes. I did take a look at that PR and instead of trying to wade through and understand all the goals and limitations of that PR I tried to start from basics and isolate to one package at a time. I opened this PR to field opinions/response on what I might be doing incorrectly or steps to test the change I might be missing.

For testing, I ran the unit tests is all. + manually verify the esm dir was created and the assets are being copied over correctly.

@aruniverse
Copy link
Member

For testing, I ran the unit tests is all

If you didnt change anything, then I imagine the tests are continuing to run against cjs and not actually testing against esm

@@ -24,7 +24,7 @@ import { HubMock } from "../../HubMock";
import { RebaseChangesetConflictArgs, TxnArgs } from "../../internal/ChangesetConflictArgs";
import { IModelTestUtils, TestUserType } from "../IModelTestUtils";
chai.use(chaiAsPromised);
import sinon = require("sinon"); // eslint-disable-line @typescript-eslint/no-require-imports
import * as sinon from "sinon";
Copy link
Contributor

@hl662 hl662 Jan 23, 2025

Choose a reason for hiding this comment

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

I would import the individual APIS from sinon, rather than import * (I was able to do so in my draft PR on ESM)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@hl662
Copy link
Contributor

hl662 commented Jan 23, 2025

is this really all it took to convert core-backend to run in esm? how did you test this?

please review changes from #7293

the important thing is we need to test an app that consumes the esm version of core-backend (without falling back to CJS). Easiest (not really) would be DTA since IModelHost is used in there, else, find a ESM-supporter backend service that can consume it and test locally

@anmolshres98 anmolshres98 marked this pull request as draft February 11, 2025 17:33
@anmolshres98
Copy link
Contributor Author

Update so far:

Locally building properly and correctly outputting the esm however rush build fails for one test package with following error:

==[ FAILURE: 1 operation ]=====================================================

--[ FAILURE: example-code-snippets ]-------------------------[ 5.26 seconds ]--


Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './lib/cjs/test/IModelTestUtils' is not defined by "exports" in /Users/anmol.shrestha/Documents/Bentley/itwinjs-core/example-code/snippets/node_modules/@itwin/core-backend/package.json
    at exportsNotFound (node:internal/modules/esm/resolve:303:10)
    at packageExportsResolve (node:internal/modules/esm/resolve:650:9)
    at resolveExports (node:internal/modules/cjs/loader:591:36)
    at Function.Module._findPath (node:internal/modules/cjs/loader:668:31)
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:1130:27)
    at Function.Module._load (node:internal/modules/cjs/loader:985:27)
    at Module.require (node:internal/modules/cjs/loader:1235:19)
    at require (node:internal/modules/helpers:176:18)
  ...5 lines omitted...
    at cjsLoader (node:internal/modules/esm/translators:356:17)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:305:7)
    at ModuleJob.run (node:internal/modules/esm/module_job:218:25)
    at ModuleLoader.import (node:internal/modules/esm/loader:329:24)
    at importModuleDynamicallyWrapper (node:internal/vm/module:431:15)
    at formattedImport (/Users/anmol.shrestha/Documents/Bentley/itwinjs-core/common/temp/node_modules/.pnpm/[email protected]/node_modules/mocha/lib/nodejs/esm-utils.js:9:14)
    at Object.exports.requireOrImport (/Users/anmol.shrestha/Documents/Bentley/itwinjs-core/common/temp/node_modules/.pnpm/[email protected]/node_modules/mocha/lib/nodejs/esm-utils.js:42:28)
    at Object.exports.loadFilesAsync (/Users/anmol.shrestha/Documents/Bentley/itwinjs-core/common/temp/node_modules/.pnpm/[email protected]/node_modules/mocha/lib/nodejs/esm-utils.js:100:20)
    at singleRun (/Users/anmol.shrestha/Documents/Bentley/itwinjs-core/common/temp/node_modules/.pnpm/[email protected]/node_modules/mocha/lib/cli/run-helpers.js:125:3)
    at Object.exports.handler (/Users/anmol.shrestha/Documents/Bentley/itwinjs-core/common/temp/node_modules/.pnpm/[email protected]/node_modules/mocha/lib/cli/run.js:370:5)

Pulled the built esm files into a local backend but was getting this error, my suspicion is that the consuming package's tsconfig needs to be updated:

import { IModelDb, IModelHost, SnapshotDb } from "@itwin/core-backend";
         ^^^^^^^^
SyntaxError: The requested module '@itwin/core-backend' does not provide an export named 'IModelDb'
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:132:21)
    at ModuleJob.run (node:internal/modules/esm/module_job:214:5)
    at ModuleLoader.import (node:internal/modules/esm/loader:329:24)
    at importModuleDynamicallyWrapper (node:internal/vm/module:431:15)
    ...(omitted)... 

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