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] Typescript Support #40

Merged
merged 3 commits into from
Feb 5, 2019
Merged

[Fix] Typescript Support #40

merged 3 commits into from
Feb 5, 2019

Conversation

BlackFenix2
Copy link
Collaborator

I have modified the .babelrc config to target the current version of node. i have been able to resolve the issue referenced here: #17. I believe the issue was that the current babel config of the package converted the store class into a function, this would not be an issue if the consuming application was also using babel to transpile their code but a typescript implementation without babel would throw a typeerror since the client passed in a class rathen than a function transpiled from a class to be called like a function. tough to explain but i think i fixed it.

old:

 _this = _possibleConstructorReturn(this, (_getPrototypeOf2 = _getPrototypeOf(store)).call.apply(_getPrototypeOf2, [this].concat(args)));

new:

 store = class extends store {
      constructor(...args) {
        super(...args);
        spy(this, config);
      }
    };

i have tested this branch with the 3 examples and my local portfolio with and without babel plugins(typescript).

i also removed the babel preset "@babel/preset-react" since there is no JSX syntax in the src files. Please let me know if you need to target a specific ECMA version, or if you need anymore details.

@BlackFenix2
Copy link
Collaborator Author

@zalmoxisus off-topic, im adding your project to Awesome Mobx here: https://github.com/mobxjs/awesome-mobx. your time-travel debugging project should make it past the curators, imma submit a PR on their branch and add your project as a devtool reference.

@zalmoxisus
Copy link
Owner

zalmoxisus commented Feb 4, 2019

Thanks for the work here and for adding to awesome-mobx! I hope I'll get 3.0 version of teh extension soon, which is supposed to work better, with new features and new ui.

About the PR, I'm confused about targeting current node version for babel. I'm usually using Node 10, but that could break some use cases, like classes not being supported by some browsers. I could use an earlier version, but we could then just specify that explecitely, and I guess that wouldn't fix our issue then.

I'd publish it as 0.4.0-alpha since it's a breaking change, then if everything is ok we could get a new release. But maybe we could still keep common-js version in lib and es version in src and make typescript use it without being transpelled through babel?

@BlackFenix2
Copy link
Collaborator Author

I targeted the current version of node to prevent the class in dev.js from being transpiled into a function which would throw an type exception if the client did not transpile their MobX classes into functions by targeting a different es version (or the client wanted to use esnext in tsconfig.json/not use babel transpilers).

I figured if the client needed to target a specific node version they can do so with their babel configuration, the client's build process would transpile our lib files to target older browsers and convert both the store class in dev.js and the Mobx Store classes provided by the client into functions.

I believe the issue is that the store class in dev.js is being transpiled into a function, not allowing clients who target newer es versions to utilize our package (example: not converting their classes to functions).

i'm sorry if this sounds confusing. Let me know what browser/node versions you intend to target and i can update this PR and find a middle ground that allows TypeScript to work without babel.

full disclosure, my tsconfig module/targets are all esnext. i use code-splitting in my app to keep the initial bundle load down.

@zalmoxisus
Copy link
Owner

zalmoxisus commented Feb 4, 2019

I understand that babel targets my (or other maintainer's) nodejs version when I publish the package (running build:lib script). So if a maintainer has nodejs bellow 4.0, classes would be transpiled, if above they won't. We could probably restrict node version in package.json, but specifying it explicitely in babel config would be better.

I would still prefer to keep a commonjs version with classes transpiled for those who need it. So we might need two directories, one with cjs and another es6 (if it differs from src)?

@BlackFenix2
Copy link
Collaborator Author

yeah, we might have to use 2 directories. for babelrc it might be better to target commonjs than set a node version.

ill see what i can do with Babel/Webpack to output cjs and es6 directories.... i actually never attempted this before, so any guidance would be helpful?

@BlackFenix2
Copy link
Collaborator Author

BlackFenix2 commented Feb 4, 2019

@zalmoxisus I added another commit to attempt to allow es6/commonjs target support, i just added the line "module": "src/index.js" in package.json and reverted my change to .babelrc to allow commonjs transpilation.

ffa7bca

the src files are es6 enough, let me know if you need anymore details, i used this article for reference:
http://2ality.com/2017/07/npm-packages-via-babel.html

also...
mobxjs/awesome-mobx#71

@zalmoxisus
Copy link
Owner

zalmoxisus commented Feb 5, 2019

That looks great! Thanks again! Hope TS will pick what is indicated in module. That is great to have anyway.

I published it as 0.3.4.

@zalmoxisus zalmoxisus merged commit 4033c34 into zalmoxisus:master Feb 5, 2019
@BlackFenix2
Copy link
Collaborator Author

just tried it out with my portfolio. i got remotedev to work in typescript without any Babel or Type Errors

would that consider this issue reoslved?: #17

@BlackFenix2 BlackFenix2 mentioned this pull request Feb 5, 2019
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.

2 participants