search

microsoft / tslib

Runtime library for TypeScript helpers.
BSD Zero Clause License
1.26k stars 128 forks source link

Update package.json: changed pattern "./" to "./*" #135

Closed NeizanSenpai closed 2 years ago

NeizanSenpai commented 3 years ago

On Angular when executes command 'ng build --prod':

NeizanSenpai commented 3 years ago

I sent this change to fix the deprecation on new apps, this is a minor change. I don't undestand why test CI for 14.x failed.

orta commented 3 years ago

This can be rebased for the CI fix 👍🏻

Neizan93 commented 3 years ago

On Angular 11 + Node v15

On Angular when executes command 'ng build --prod':

  • Generating browser application bundles...(node:16716) [DEP0148] DeprecationWarning: Use of deprecated folder mapping "./" in the "exports" field module resolution of the package at {{APP_PATH}}\node_modules\tslib\package.json. Update this package.json to use a subpath pattern like "./*". (Use node --trace-deprecation ... to show where the warning was created)
Graphmaxer commented 3 years ago

Any update on this ? @Neizan-93 Can you rebase on master and after can we merge this. I'm having this warning with node 15.

rbuckton commented 3 years ago

I just closed #139 as a duplicate of this. In #139, the author points to postcss/postcss#1455, which tried to solve the deprecation in the same way. From the linked PR it sounds like this breaks NodeJS 12.x and there are still issues with it:

@weswigham any thoughts?

rbuckton commented 3 years ago

@orta We are skipping esm node modules for NodeJS 12, so we're not seeing a possible error due to the fact NodeJS 12 supports subpath exports but not subpath patterns.

rbuckton commented 3 years ago

I've done some research and cannot can verify the issue mentioned in https://github.com/postcss/postcss/issues/1455#issuecomment-722502548. With "./*": "./*", NodeJS 12.7 seems to work just fine (as long as --experimental-modules are passed to NodeJS), but NodeJS 12.18 (and possibly other versions I have not yet tested) have issues.

Update: NodeJS 12.20.1 seems to work fine with `"./": "./"`, so it seems like they addressed this inconsistency in a patch.

The test case I used was:

// index.mjs
async function main() {
    // (a)
    try {
        const tslib1 = await import("tslib");
        console.log("default" in tslib1 ? "CommonJS" : "ESM");
    }
    catch (e) {
        console.error(e.name, e.message);
    }

    // (b)
    try {
        const tslib2 = await import("tslib/tslib.js");
        console.log("default" in tslib2 ? "CommonJS" : "ESM");
    }
    catch (e) {
        console.error(e.name, e.message);
    }

    // (c)
    try {
        const tslib3 = await import("tslib/tslib.es6.js");
        console.log("default" in tslib3 ? "CommonJS" : "ESM");
    }
    catch (e) {
        console.error(e.name, e.message);
    }

    // (d)
    try {
        const tslibpkg = await import("tslib/package.json");
        if (tslibpkg.default.name !== "tslib") throw new Error ("Unexpected result");
        console.log("ok");
    }
    catch (e) {
        console.error(e.name, e.message);
    }
}

main();

Results:

Version Mapping (a) (b) (c) (d)
12.7.0 "./" CommonJS CommonJS SyntaxError 1 ok
12.7.0 "./*"" CommonJS CommonJS SyntaxError 1 ok
12.18.3 "./" ESM CommonJS SyntaxError 1 TypeError 2, 3 / ok 4
12.18.3 "./*" ESM Error 5 Error 5 Error 3, 4, 5
12.20.1 "./" ESM CommonJS SyntaxError 1 TypeError 2, 3 / ok 4
12.20.1 "./*" ESM CommonJS SyntaxError 1 TypeError 2, 3 / ok 4
15.4.0 "./*" ESM CommonJS SyntaxError 1 TypeError 2, 3 / ok 4

NOTES:

  1. SyntaxError Unexpected token export - Loader tried to load ESM module as CommonJS
  2. TypeError Unknown file extension ".json" - Loader won't load .json without --experimental-modules
  3. Without --experimental-modules
  4. With --experimental-modules
  5. Error Package subpath '?' is not defined by "exports" - Loader won't load any subpath.

The most common use case for tslib is to just import the default export: Playground link. That means:

However, subpath exports are trickier:

The only consistency between 12.18 and 15.4 is "./", switching to "./*" would break anyone still using NodeJS 12.18 and subpath exports. At this time I'm not sure how impactful that break would be. NodeJS 12.20 seems to handle things correctly.

Since NodeJS 12 is a Maintenance LTS, is our stance to only support the latest version of a Maintenance LTS? If so, then this change is probably fine.

orta commented 3 years ago

@orta We are skipping esm node modules for NodeJS 12,

Interesting, I didn't know it was supported in 12 TBH.

I think it's pretty reasonable to assume here that the 12.18.x versions had a bug and we can add release notes stating they can update to a later version of 12.x to fix issues?

rbuckton commented 3 years ago

It was experimental in 12. I think this change is fine.

brunogrcsada commented 3 years ago

This really needs to be merged if possible @JonSenpai @rbuckton, there are quite a few dependencies that rely on tslib, and currently I'm facing a series of deprecation warnings during every build. I can just suppress it but it's not ideal :(

csvn commented 3 years ago

This PR should solve #152. Is there anything still blocking this from being merged?

Sidenote: Why is the ./* export even needed? All .js files in this repository are already being referenced by the conditional exports in the . path. Is there some other file that dependencies require from tslib?

RVledder commented 2 years ago

@csvn interesting approach using both syntaxes or exporting only the package.json.

Running the test script from @rbuckton on the current node version (17):

Version Mapping (a) (b) (c) (d)
17.0.0 "./": "./" (current implementation) ESM Error ⁵ Error ⁵ Error ⁵
17.0.0 "./package.json": "./package.json" ESM Error⁵ Error ⁵ TypeError ²
17.0.0 none ESM Error ⁵ Error ⁵ TypeError ²
17.0.0 "./*": "./*" ESM CommonJS SyntaxError ¹ TypeError ²

Seems like "./*": "./*" is a possible solution. But I'm not able to see why the CI has failed on the third job (14.x) since logs for this run have expired. And there might be some caveats (see my comment)

Made a proposal with explicitly defining the files here: https://github.com/microsoft/tslib/pull/163

Note: Came across this thread after I created the PR and wasn't aware of the test script. Will run the same tests against that solution also, and post the results in the PR.

CobusKruger commented 2 years ago

@rbuckton do we know when the next release will be? It seems the last one was in August.

phocks commented 2 years ago

Current code is: "./*": "./*", "./": "./"

VS Code seems to still complain, saying "Property ./ is not allowed."

Maybe should just be "./*": "./*" ?? ie. delete the last line https://github.com/microsoft/tslib/blob/c827964226e85118e2fd35b1cc68d4a5ad867f39/package.json#L36 (and the preceding comma)

weswigham commented 2 years ago

./ is for old node, it's deprecated in new node, and thus not allowed by the json schema. It's safe to ignore. (New node will always match the * containing case first and so should never hit the deprecation warning for still having ./).

phocks commented 2 years ago

./ is for old node, it's deprecated in new node, and thus not allowed by the json schema. It's safe to ignore. (New node will always match the * containing case first and so should never hit the deprecation warning for still having ./).

Ah yes, now I get it. It's for backwards compatability. I'll ignore the VS Code warning. Thanks. Please ignore me :)