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

[WRONG] jsc.experimental.keepImportAssertions is experimental, explicitly #7923

Closed
meabed opened this issue Sep 7, 2023 · 19 comments · Fixed by #7995
Closed

[WRONG] jsc.experimental.keepImportAssertions is experimental, explicitly #7923

meabed opened this issue Sep 7, 2023 · 19 comments · Fixed by #7995
Labels

Comments

@meabed
Copy link

meabed commented Sep 7, 2023

Describe the bug

The bug is described here: TypeStrong/ts-node#2056

With version 1.3.83, swc renames the experimental configuration option jsc.experimental.keepImportAssertions to jsc.experimental.keepImportAttributes as per #7914

Suggestion to keep other libraries working without so much failure:

  • Adding jsc.experimental wrong or non-supported keys shouldn't throw an error ( maybe warning )
  • Alias renamed keys to the new keys to it still backward compatible

The error during running ts-node:

Failed to deserialize buffer as swc::config::Options
JSON: {"sourceMaps":true,"module":{"noInterop":false,"type":"commonjs","strictMode":true,"ignoreDynamic":false},"swcrc":false,"jsc":{"parser":{"syntax":"typescript","tsx":false,"dynamicImport":true,"importAssertions":true},"target":"es2015","transform":{"legacyDecorator":true,"react":{"throwIfNamespace":false,"useBuiltins":false}},"keepClassNames":false,"experimental":{"keepImportAssertions":true}}}

Caused by:
    unknown field `keepImportAssertions`, expected one of `plugins`, `keepImportAttributes`, `emitAssertForImportAttributes`, `cacheRoot`, `disableBuiltinTransformsForInternalTesting` at line 1 column 396

Input code

No response

Config

No response

Playground link

No response

SWC Info output

No response

Expected behavior

The build should be working normally

Actual behavior

No response

Version

1.3.83

Additional context

No response

@meabed meabed added the C-bug label Sep 7, 2023
@kdy1
Copy link
Member

kdy1 commented Sep 7, 2023

keepImportAssertions is explixitly marked as an experimental option. Please read docs.

@kdy1 kdy1 closed this as not planned Won't fix, can't repro, duplicate, stale Sep 7, 2023
@meabed
Copy link
Author

meabed commented Sep 7, 2023

@kdy1 I understand its experimental, what is the harm of throwing a warning instead of error?

@kdy1
Copy link
Member

kdy1 commented Sep 7, 2023

It gives a good impression to people who don't understand the word experimental

@kdy1 kdy1 changed the title Renaming this jsc.experimental.keepImportAttributes shouldn't throw an error jsc.experimental.keepImportAssertions is experimental, explicitly Sep 7, 2023
@kdy1 kdy1 changed the title jsc.experimental.keepImportAssertions is experimental, explicitly [WRONG] jsc.experimental.keepImportAssertions is experimental, explicitly Sep 7, 2023
@meabed
Copy link
Author

meabed commented Sep 7, 2023

@kdy1 I understand, although I think warning gives the same impression, error is more obstructive action and not as helpful.

@cpojer
Copy link

cpojer commented Sep 8, 2023

@kdy1 I understand your explanation regarding the option being experimental, yet it appears that ts-node, for some reason, chose to make use of this configuration option. By changing this in swc in a minor patch you are breaking any user of ts-node + swc. Users do not have control over a fix until ts-node merges this PR: https://github.com/TypeStrong/ts-node/pull/2057/files

I've been advertising for this approach on my blog and expect it is quite commonly used. I don't know how long it will take for ts-node to publish a new release, but it would be great if you could revert this change until ts-node has published an update to their project.

@dandrei
Copy link

dandrei commented Sep 9, 2023

My previously-working ts-node installation is now broken after installing the latest @swc/core version.
Reverting to @swc/[email protected] explicitly fixes the issue.

@josser
Copy link

josser commented Sep 18, 2023

Anyone can help with the quickfix for this?
I don't have keepImportAssertions in any configuration files. Is It somewhere in ts-node internals or so?

@kdy1
Copy link
Member

kdy1 commented Sep 18, 2023

@josser The fix is already published as v1.3.84.

https://github.com/swc-project/swc/blob/f66da772202a6f065488fbabb7e0a9d9a2d7089d/CHANGELOG.md#1384---2023-09-11

I won't add an alias to the deprecated experimental option, though.

@vluoto
Copy link

vluoto commented Sep 19, 2023

@cpojer
Copy link

cpojer commented Sep 19, 2023

ts-node already has the necessary fixes on the main branch, we just need a new release.

@meabed
Copy link
Author

meabed commented Sep 23, 2023

Thank you @kdy1, I really think supporting the community to transition smoothly should be more important than debating this issue resolution, but as you could see the fix may or may not land soon in ts-node, and I know everyone will appreciate your support and flexibility on this issue.

And I want to highlight the same approach in nextjs for example when you configure experimental features and they change or get removed - it shows warning and not error.

[0] $ next start
[0]  ⚠ Invalid next.config.js options detected: 
[0]  ⚠     The value at .experimental has an unexpected property, runtime, which is not in the list of allowed properties (appDocumentPreloading, adjustFontFallbacks, adjustFontFallbacksWithSizeAdjust, allowedRevalidateHeaderKeys, amp, clientRouterFilter, clientRouterFilterRedirects, clientRouterFilterAllowedRate, cpus, memoryBasedWorkersCount, craCompat, caseSensitiveRoutes, useDeploymentId, useDeploymentIdServerActions, deploymentId, disableOptimizedLoading, disablePostcssPresetEnv, esmExternals, serverActions, serverActionsBodySizeLimit, extensionAlias, externalDir, externalMiddlewareRewritesResolve, fallbackNodePolyfills, fetchCacheKeyPrefix, forceSwcTransforms, fullySpecified, gzipSize, incrementalCacheHandlerPath, isrFlushToDisk, isrMemoryCacheSize, largePageDataBytes, manualClientBasePath, middlewarePrefetch, nextScriptWorkers, optimizeCss, optimisticClientCache, outputFileTracingRoot, outputFileTracingExcludes, outputFileTracingIgnores, outputFileTracingIncludes, ppr, proxyTimeout, serverComponentsExternalPackages, scrollRestoration, sharedPool, sri, strictNextHead, swcMinify, swcPlugins, swcTraceProfiling, urlImports, workerThreads, webVitalsAttribution, mdxRs, typedRoutes, webpackBuildWorker, turbo, optimizePackageImports, optimizeServerReact, instrumentationHook, turbotrace, logging, serverMinification, serverSourceMaps).
[0]  ⚠ See more info here: https://nextjs.org/docs/messages/invalid-next-config
[0]   ▲ Next.js 13.5.2

image

I am aware that you work in vercel's team and sharing the principles of supporting the community to transition to better software, I think this solution is good balanced approach in ensuring library users knows what is the issue and not stopping their workflow or stuck in old version because of it.

@kdy1
Copy link
Member

kdy1 commented Sep 23, 2023

I added an alias with #7995

@kdy1
Copy link
Member

kdy1 commented Sep 23, 2023

Honestly, I'm really against such PR, but I don't want to think/debate about it

@meabed
Copy link
Author

meabed commented Sep 23, 2023

Thank you @kdy1 appreciate it.

kdy1 added a commit that referenced this issue Sep 23, 2023
@meabed
Copy link
Author

meabed commented Sep 24, 2023

Not sure if it's related, but After updating to "@swc/core": "1.3.88"
I get another error with noInterop

JSON: {"sourceMaps":true,"module":{"noInterop":false,"type":"es6","strictMode":true,"ignoreDynamic":false},"swcrc":false,"jsc":{"parser":{"syntax":"typescript","tsx":false,"dynamicImport":true,"importAssertions":true},"target":"es2015","transform":{"legacyDecorator":true,"react":{"throwIfNamespace":false,"useBuiltins":false}},"keepClassNames":false,"experimental":{"keepImportAssertions":true}}}

Caused by:
    unknown field `noInterop`, expected `resolveFully` at line 1 column 391

I have opened ts-node issue to remove it TypeStrong/ts-node#2070

@kdy1
Copy link
Member

kdy1 commented Sep 24, 2023

@meabed #7963

@meabed
Copy link
Author

meabed commented Sep 24, 2023

Thanks @kdy1

Curios question is there away to replace ts-node with swc fully with type checking?

@cjshearer
Copy link

Thanks @kdy1

Curios question is there away to replace ts-node with swc fully with type checking?

swc doesn't do typechecking, although there is an experimental project to rewrite the TypeScript checker in rust: https://github.com/dudykr/stc (very experimental)

@swc-bot
Copy link
Collaborator

swc-bot commented Oct 26, 2023

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@swc-project swc-project locked as resolved and limited conversation to collaborators Oct 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging a pull request may close this issue.

8 participants