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 typescript emit path if --cwd is used #643

Closed
katywings opened this issue May 31, 2020 · 13 comments
Closed

Wrong typescript emit path if --cwd is used #643

katywings opened this issue May 31, 2020 · 13 comments

Comments

@katywings
Copy link
Collaborator

IS: If you use the cli cwd option to build a nested package xyz, the types are emitted into ./dist/.

SHOULD: Types should be emitted into the dist folder xyz/dist of the nested package.

DEMO:

REMARKS:

  • If the devtools/lib/main.ts doesnt import something from an external, the ts emit path is different but still wrong: dist/main.d.ts
  • Not directly related to the cwd thing, but I noticed that the --external cli option "freaks out" if the external has a dash character: microbundle prints: No name was provided for external module 'microbundle-test' in output.globals – guessing 'microbundleTest'. Doing the exact same bundle but naming the global "microbundletest" doesnt print this warning. (P.S. if you try this you also have to change the tsconfig.json path alias from microbundle-test to microbundletest)
@developit
Copy link
Owner

developit commented May 31, 2020

Ahh good point about external - we should be pre-populating the name when generating entry point externals but we aren't!

@developit
Copy link
Owner

Ah I found the cwd issue - we pass the package.json types path directly to rollup-plugin-typescript2, but should be resolving it against the cwd option value.

@katywings
Copy link
Collaborator Author

katywings commented Jun 2, 2020

@marvinhagemeister
Copy link
Collaborator

marvinhagemeister commented Jun 2, 2020

Fixed by #646 🎉

@katywings
Copy link
Collaborator Author

katywings commented Jun 2, 2020

@marvinhagemeister Did you see my comment: #643 (comment) ?

There is actually one topic in this issue that wasnt fixed by my pr: #643 (comment) 😅

@marvinhagemeister
Copy link
Collaborator

@katywings oh I acted too quickly here. Sorry, that's on me 🙈

@developit
Copy link
Owner

@katywings I wonder - would it be possible for Microbundle to just generate that proxy types file with the correct name? I'd love to keep things as close to zero-config as possible.

@katywings
Copy link
Collaborator Author

katywings commented Jun 3, 2020

@developit The problem is that typescript emits the types based on the structure of the source files including tsconfig path based "aliases".

Say you have a structure like this:

  • preact/lib/main.ts (importing X from '../../utils.ts')
  • utils.ts

Then the emit will be like

  • preact/lib/main.d.ts

Because tsc tries to account for the ".." things


I cannot think of a non over-engineering solution which could automatically account for the different possible file structures 🤔

Edit: This is how I automatically created type proxies in witney before the large microbundle rework. But such an automatism only works if you force the consumers into a specific lib file structure ^^.

@katywings
Copy link
Collaborator Author

katywings commented Jun 3, 2020

I kept thinking about what microbundle would need to do, to get correct dist types in all situations, but as I mentioned it, this is a very over engineered solution:

  • Delete old dist (ideally via some parameter)
  • Bundle as normal but add suffix "types" to declarationDir
  • Detect if [source] has a default export
  • Detect if [source] has other exports
  • If dist/types/[sourcePathWithDTsEnding] exists: create type proxy file [pkg.types] that exports stuff from prev detection from dist/types/[sourcePathWithDTsEnding]
  • else: create type proxy file [pkg.types] that exports stuff from prev detection from dist/types/[basename(pkg.types)]

I hope its clear what I mean, let me know otherwise xD.

@katywings
Copy link
Collaborator Author

@developit I think I now understand, why microbundle-test as external showed the warnig No name was provided for external module 'microbundle-test' in output.globals – guessing 'microbundleTest, while microbundle as external didnt:

if (name.match(/^[a-z_$][a-z0-9_$]*$/)) {

Normally the externals are directly passed into globals, but this regex pattern [a-z0-9_$] doesn't accept the dash - character and so microbundle-test will not be included in the globals :D. What do you think about adding - to the regex as valid character?

This was referenced Jun 16, 2020
@developit
Copy link
Owner

developit commented Jun 19, 2020

@katywings the trick there is that when something ends up in globals, Rollup will try to reference it from this/window:

import test from 'microbundle-test';
+
globals['microbundle-test'] = 'microbundle-test';

...will output:

(function(factory, global) {
    if (typeof define=='function' && define.amd) define(['microbundle-test'], factory);
    else if (typeof module != 'undefined') module.exports = factory(require('microbundle-test'));
    else global.foo = factory(global['microbundle-test']);
})(function(test){
}, this)

Most libraries use camelCase for their global variable names rather than dash-case, since it's the default in both Rollup and Webpack.

@katywings
Copy link
Collaborator Author

@developit The only thing still "open" in this thread is the topic about typescript proxies (#643 (comment))

Since I think that this would need a lot over engineering, I gonna close this issue :) - you can reopen if you think this really should be worked out in microbundle.

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

No branches or pull requests

3 participants