-
Notifications
You must be signed in to change notification settings - Fork 508
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): added rollup plugin @zerollup/ts-transform-paths, injected as … #374
Conversation
my monorepo build with fixed version: |
ok nice work, think this one needs @jaredpalmer to look at it before we merge |
yep) meanwhile I've checked official |
…transformer in rollup-plugin-typescript2 config
033ed38
to
1750c10
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a test to ensure this works? Would require running a compiled module that imports something out of src/
to check that it was actually imported. A bit convoluted.
Beyond that, I wonder if this is handled by the parcel
playground and storybook
template correctly, or if support needs to be added for those too. Not sure, but those can be added later if needed
@@ -50,6 +51,9 @@ export async function createRollupConfig(opts: TsdxOptions) { | |||
tsconfigJSON = fs.readJSONSync(resolveApp('tsconfig.json')); | |||
} catch (e) {} | |||
|
|||
const pathTransformer = (service: any): any => | |||
transformPaths(service.getProgram()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these be more specific than any
? transformPaths
is written in TS, so I think it can be.
Also do you know if the second arg isn't necessary? It has an exclude
option, not sure if we should pass tsconfigJSON.exclude
into there or if it's read automatically (and the exclude
option is just an override maybe?? idk the docs aren't comprehensive)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, yea) transformPaths
has types LanguageService and CustomTransformer - need to use,
to be fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've build my lib with patched version with paths config, and then check with simple react app - no problem found
but fully agree - need to check any possible combinations and add tests
to be implemented)
sorry did not mean to close |
Questions:
|
mm.. yeah, I'll check possible solution with babel
I already use lerna monorepo + tsdx (my patched version with ts-paths plugin) in each package + test CRA app + |
@jaredpalmer @agilgur5 we can avoid use of transformers in rpts2 and use |
OMG.. tleunen/babel-plugin-module-resolver#336 (comment) |
Useful info: https://mitchellsimoens.com/2019/08/07/why-typescript-paths-failed-me/
|
@agilgur5 @jaredpalmer proof with 'storybook' template & |
So my question is if this should be part of the tsdx out of the box. Is this really that common of a thing? Seems like it is niche? Someone could easily just publish a “plugin” that adds this and we could link to it |
@jaredpalmer I know many developers who use aliases plugins to deal with "../../../" hell.. But also agree that adding yet another plugin for plugin for plugin in the core is not excellent idea.. so.. I reject my PR and add comment to HOWTOs how to setup aliasing (@sw-yx it would be good to pin HOWTOs issue.. :) |
So just to summarize:
So In either case, it seems like trying to resolve paths is more of a hassle than worthwhile, and potentially a very fragile feature, so I think I'd agree that it should be left to user-land as a result. That being said, as it is a common use case, I do think there should be an easier way to configure it. As |
Copy/pasta snippet incoming for people in need: // babel.config.js
module.exports = {
// ...
plugins: [
// ... other plugins
[
'babel-plugin-module-resolver',
{
root: './',
alias: require('./tsconfig.json').compilerOptions.paths,
},
],
],
}; This works wonderfully in combination with |
thanks @0vidiu, added to HOWTOs #379 (comment) |
thanks @0vidiu @ambroseus This workaround works fine for the JS output, but the typedefs are emitted in different files, and they still have the absolute links, making the typedef files unable to resolve each others. Any idea how to solve it? |
The https://github.com/zerkalica/zerollup/tree/master/packages/ts-transform-paths transformer should handle type files properly from what their docs say |
It seems not working at "babel-plugin-module-resolver": "^4.1.0", and can be fixed by changing babel.config.js as follow. module.exports = {
plugins: [
[
'module-resolver',
{
alias: {
"@": "./src"
}
},
],
],
} the key of resolving this problem is changing alias value from array to string. |
…transformer in rollup-plugin-typescript2 config
closes #336
decided to not fix
tsdx
configs but provide info for solution: #379 (comment)Edit/Summary From @jaredpalmer:
When using tsdx with larger packages, it is useful to use typescript path aliasing to avoid relative imports. This change allows tsdx (rollup and typescript) to respect such options.
Before
After