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

Force ES5 for TS compilation #250

Merged
merged 1 commit into from Jul 2, 2020
Merged

Force ES5 for TS compilation #250

merged 1 commit into from Jul 2, 2020

Conversation

lmiller1990
Copy link
Member

This prevents const vue_1 = require('vue') from conflicting, since it will do var vue_1 instead.

This feels pretty bad, I don't know the correct solution. 🤔

@lmiller1990 lmiller1990 changed the base branch from master to next July 2, 2020 14:02
@lmiller1990 lmiller1990 merged commit 4eb19bc into next Jul 2, 2020
@LinusBorg
Copy link
Member

I think we could use a pretty basic regex to change that line to a var ... somewhere around here:

if (output.includes('exports.render = render;')) {
output += ';exports.default = {...exports.default, render};'
} else {
output += ';exports.default = {...exports.default};'

@LinusBorg
Copy link
Member

This whole part is a bit brittle since we use code that is meant to end up in multiple (virtual) files and concat it into one piece of code.

If I were to have a top-level variable or function called render in my script block, the function render( generated by the template compilation step would conflict here as well, I would assume.

@lmiller1990 lmiller1990 deleted the lachlan/force-es5 branch July 2, 2020 22:23
@lmiller1990
Copy link
Member Author

lmiller1990 commented Jul 2, 2020

I would say so - I experimented a bit to try and get other variables to conflict, no luck, but I'm sure it's possible.

I am not really sure on the best way to solve this problem 🤔 Maybe we need some way to hoist up the common imports and do not duplicate them. That would not solve the top level render conflict you pointed out, though. I pinged a maintainer of ts-jest to get some ideas, he may have some experience around this.

Edit: he suggests asking the babel team: #238 (comment). I will play around a little more and see if I can think of a better approach.

@ahnpnl
Copy link

ahnpnl commented Jul 30, 2020

Hi, come back to this I have a suspicion. It seems like the error is thrown because in the final js output which is given to jest containing twice const vue_1 = require(‘vue’) which will then cause the issue cannot redeclare block-scope variable.

If that is the case, the const vue_1 = require(‘vue’) needs to be removed from vue-jest transpile output by either of these ways:

  • Use regex
  • Write a Babel plug-in, which is also an AST transformer to use with Babel.
  • Write a TypeScript transformer, which is also AST transformer to use with TypeScript API.

I’d recommend to use AST transformer over regex when dealing with transpiling codes

@lmiller1990
Copy link
Member Author

I think you are right - using var is just a work around, not really a good solution.

I think that we should make a new codebase for a TypeScript and ts-jest based solution that implements the best patterns that I learned from looking at other jest transformers, and pin it to Jest versions like ts-jest does - supports both Vue 2.x and 3.x, and multiple jest versions, is very difficult in a single codebase.

@ahnpnl
Copy link

ahnpnl commented Jul 31, 2020

ya, agree. Maybe you can setup the base structure 1st and others can start contributing, also worth to write down in an issue about all the plans :)

@lmiller1990
Copy link
Member Author

Yep, we definitely need a good "plan of attack".

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

3 participants