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

Fix TS 4.8+ #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix TS 4.8+ #1

wants to merge 1 commit into from

Conversation

bradennapier
Copy link

@bradennapier bradennapier commented Oct 25, 2022

Typescript 4.8 was not working for me so as I investigated I came to conclusion it was this file causing the issue due to their recent deprecation of decorators.

These changes seem to satisfy the requirements , although I wrote it in bed on my cell via github.com and just got up to test it and it built our huge project (thousands of ts files in a monorepo with references and such) and seems to work.

Have only worked with ts compiler a bit though so...

I am most wary about the change from createLiteral to createStringLiteral - I am not sure if it can be any other value as I imagine that is the string but there are a ton of things like createLiteralFalse and such so may need a switch statement there ?

Typescript 4.8 was not working for me so as I investigated I came to conclusion it was this file causing the issue due to their recent deprecation of decorators. 

These changes seem to satisfy the requirements , although I wrote it on my cell phone via github.com so styling and such may now be best...
@emiljanitzek
Copy link

We also had problems when upgrading TS > 4.7. @falkenhawk is it possible to see if we can merge this fix?

@falkenhawk
Copy link
Member

Thank you @bradennapier for the PR and @emiljanitzek for the ping! I somehow missed it. I'll look into it tomorrow if time allows.

@bradennapier
Copy link
Author

Thank you

@falkenhawk
Copy link
Member

@bradennapier is it potentially a BC-breaking change? Should we bump typescript dependency to 4.8+? Or will it still work with older versions of typescript? 🤔
Currently ts peer-dependency is defined as follows:

  "peerDependencies": {
    "typescript": ">=3.7.2"
  },

https://github.com/ovos/zerollup/blob/fix/typescript-4.5/packages/ts-transform-paths/package.json#L36-L38

@bradennapier
Copy link
Author

Honestly I am not sure lol, but I would say its not a bad idea?

@bradennapier
Copy link
Author

@falkenhawk Upon review, I do not believe it has breaking changes, although with the significant changes with decorators coming up now may be a good time to bump the dep.

Would love to get this merged if it checks out for you though!

@falkenhawk
Copy link
Member

I've tried to build the package locally, with your changes applied and got Error: (...)packages/ts-transform-paths/src/importPathVisitor.ts(67,22): semantic error TS2339: Property 'factory' does not exist on type 'typeof ts'. 🤔

❯ npx cross-env BUILD_PKG=ts-transform-paths npm run build

> zerollup@1.0.0 build
> rollup -c rollup.config.js


(...)packages\ts-transform-paths\src\index.ts → packages\ts-transform-paths\dist\index.mjs, packages\ts-transform-paths\dist\index.js...
(!) Unresolved dependencies
https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency
@zerollup/ts-helpers (imported by packages\ts-transform-paths\src\transformPaths.ts, packages\ts-transform-paths\src\ImportPathInternalResolver.ts)
[!] (plugin rpt2) Error: (...)packages/ts-transform-paths/src/importPathVisitor.ts(67,22): semantic error TS2339: Property 'factory' does not exist on type 'typeof ts'.
packages\ts-transform-paths\src\importPathVisitor.ts
Error: (...)packages/ts-transform-paths/src/importPathVisitor.ts(67,22): semantic error TS2339: Property 'factory' does not exist on type 'typeof ts'.
    at error ((...)node_modules\rollup\dist\shared\node-entry.js:5400:30)
    at throwPluginError ((...)node_modules\rollup\dist\shared\node-entry.js:11878:12)
    at Object.error ((...)node_modules\rollup\dist\shared\node-entry.js:12912:24)
    at Object.error ((...)node_modules\rollup\dist\shared\node-entry.js:12081:38)
    at RollupContext.error ((...)node_modules\rollup-plugin-typescript2\src\rollupcontext.ts:37:18)
    at (...)node_modules\rollup-plugin-typescript2\src\print-diagnostics.ts:41:11
    at arrayEach ((...)node_modules\rollup-plugin-typescript2\node_modules\lodash\lodash.js:530:11)
    at Function._.each [as forEach] ((...)node_modules\rollup-plugin-typescript2\node_modules\lodash\lodash.js:9410:14)
    at printDiagnostics ((...)node_modules\rollup-plugin-typescript2\src\print-diagnostics.ts:9:2)
    at Object.transform ((...)node_modules\rollup-plugin-typescript2\src\index.ts:244:5)

@bradennapier
Copy link
Author

@falkenhawk maybe wipe node modules? Factory is the property the docs had us change to so I guess that confirms we should update peer dep

@falkenhawk
Copy link
Member

@bradennapier sadly, it's not that easy. peerDepedencies are only for usage at the end. But the package needs to be built first to be published on npm.

The codebase is built with typescript >=3.7.2 <4, (or fixed exactly at 3.7.2 e.g. in root package.json), and it won't build when simply bumping dependencies to typescript v4, as the existing code is not compatible. It would require effort to upgrade the code to v4. Also, it is not only about ts-transform-paths package, as it depends on other packages from the repo, which have to be adjusted as well.

Unfortunately I don't have time to invest into this, as we have already migrated our project, where we previously used this plugin, to https://github.com/justkey007/tsc-alias (which we also had to monke-patch as it does not handle cross-dependencies between local monorepo packages...)

Would you be willing to continue here to make this package build on typescript v4? Otherwise, you might be better off simply patching it with patch-package in your project 😥

@bradennapier
Copy link
Author

@falkenhawk it turns out this a regression which makes sense since it breaks a TON of ts plugins across the board.

microsoft/TypeScript#51544

4.9.4 should fix it so the fixes that work around the regression should become unneeded (although recommended) from my understanding.

@bradennapier
Copy link
Author

Unfortunately 4.9.4 does not seem to fix it so I found myself needing to go back to { "transform": "typescript-transform-paths" }, which curiously didnt work and caused me to come to this package... but seems to work again :-P

I think 5.0 is looking into supporting paths and path rewrites potentially, but I didn't read the whole feature since its a book.

@fires3as0n
Copy link

I switched to https://www.npmjs.com/package/typescript-transform-paths, just had to edit the "paths" tsconfig setting, otherwise works the same

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

4 participants