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

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

Merged
merged 2 commits into from Feb 8, 2022

Conversation

NeizanSenpai
Copy link
Contributor

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)

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)
@NeizanSenpai
Copy link
Contributor Author

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
Copy link
Contributor

orta commented Nov 18, 2020

This can be rebased for the CI fix 👍🏻

@Neizan93
Copy link

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
Copy link

Graphmaxer commented Dec 2, 2020

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
Copy link
Member

rbuckton commented Jan 5, 2021

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
Copy link
Member

rbuckton commented Jan 5, 2021

@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
Copy link
Member

rbuckton commented Jan 5, 2021

I've done some research and cannot can verify the issue mentioned in postcss/postcss#1455 (comment). 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:

  • 12.7.0 - Fails in both cases because its importing the CommonJS file, and thus all the exports are actually on the default export.
  • 12.18.3 - Succeeds in both cases
  • 12.20.1 - Succeeds in both cases
  • 15.4.0 - Succeeds in both cases

However, subpath exports are trickier:

  • 12.7.0 - Only loads as CommonJS in both cases
  • 12.18.3
    • "./" - Only loads subpath exports as CommonJS (fails for tslib/tslib.es.js, probably due to file extension)
    • "./*" - Doesn't work at all.
  • 12.20.1
    • "./" - Only loads subpath exports as CommonJS (fails for tslib/tslib.es.js, probably due to file extension)
    • "./*" - Only loads subpath exports as CommonJS (fails for tslib/tslib.es.js, probably due to file extension)
  • 15.4.0
    • "./" - Only loads subpath exports as CommonJS (fails for tslib/tslib.es.js, probably due to file extension), shows deprecation warning
    • "./*" - Only loads subpath exports as CommonJS (fails for tslib/tslib.es.js, probably due to file extension)

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
Copy link
Contributor

orta commented Jan 6, 2021

@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
Copy link
Member

rbuckton commented Jan 6, 2021

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

@brunogrcsada
Copy link

brunogrcsada commented Jun 28, 2021

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
Copy link

csvn commented Aug 19, 2021

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?

package.json Outdated Show resolved Hide resolved
@StudioSpindle
Copy link

StudioSpindle commented Nov 24, 2021

@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: #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.

Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>
@rbuckton rbuckton merged commit 06853a8 into microsoft:main Feb 8, 2022
@CobusKruger
Copy link

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

@phocks
Copy link

phocks commented Feb 28, 2022

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

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

Maybe should just be "./*": "./*" ?? ie. delete the last line

"./": "./"
(and the preceding comma)

phocks added a commit to phocks/tslib that referenced this pull request Feb 28, 2022
Leaving ./ in there will still cause a warning I reckon as seen in microsoft#135
@phocks phocks mentioned this pull request Feb 28, 2022
@weswigham
Copy link
Member

./ 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
Copy link

phocks commented Feb 28, 2022

./ 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 :)

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.

None yet