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

Use babel to create ie11 (es5) compatible build #5619

Closed
wants to merge 1 commit into from

Conversation

nyurik
Copy link
Member

@nyurik nyurik commented Dec 4, 2019

This creates a build-es5/ dir, similar to Vega main repo.
I am not yet certain if this is required, or if tsc compiler
already creates ie11-compatible code as is.

Please:

  • Make the PR atomic. (Fix one issue at a time.) Multiple relevant issues that must be fixed together? Make atomic commits so we can easily review each issue.

  • Provide a concise title so we can easily copy it to the release note.

    • Use imperative mood and present tense.
    • Mention relevant issues. (e.g., Fix #1 / Fix part of #1)
  • Lint and test (Run yarn test)

  • Rebase onto the latest master branch

  • Review your changeset first (to ensure code quality)

  • For new features

    • Add new unit tests
    • Update the documentation under site/docs/ + add examples

Tips:

This creates a build-es5/ dir, similar to Vega main repo.
I am not yet certain if this is required, or if tsc compiler
already creates ie11-compatible code as is.
@domoritz
Copy link
Member

domoritz commented Dec 4, 2019

I would strongly prefer not to add another dependency and instead just use tsc (if possible).

@nyurik
Copy link
Member Author

nyurik commented Dec 4, 2019

I would too, but this keeps it consistent with Vega lib itself, making sure it matches exactly the same output requirements/expectations. With tsc, you will always have to match babel profile with tsc settings - not something that will remain consistent. Besides, this is not like you are including a new binary, just using some extra tool in the build chain - not something that critical.

But again, it is still unknown if this is needed, will be doing some testing soon.

@domoritz
Copy link
Member

domoritz commented Dec 4, 2019

Makes sense. Maybe we can use babel instead of TSC to transpile Vega-Lite in general.

Vega and Vega-Lite have somewhat different build systems. Vega does not use much Typescript right now for example.

@terencehonles terencehonles mentioned this pull request Feb 15, 2020
5 tasks
@terencehonles
Copy link

Makes sense. Maybe we can use babel instead of TSC to transpile Vega-Lite in general.

Vega and Vega-Lite have somewhat different build systems. Vega does not use much Typescript right now for example.

Using Babel instead of TSC would probably not be the best idea at this point in time.

According to the announcement post:

Caveats

As we mentioned above, the first thing users should be aware of is that Babel won’t perform type-checking on TypeScript code; it will only be transforming your code, and it will compile regardless of whether type errors are present. ...

@domoritz
Copy link
Member

I see. Then let's just switch to tsc only.

@domoritz domoritz closed this Feb 16, 2020
@terencehonles
Copy link

I see. Then let's just switch to tsc only.

Just to be clear, the quote I pointing out was TS -> JS via Babel, not ES6 -> ES5 via Babel, and this PR would still work. I'm not sure if we want to continue down this route or #5920

@terencehonles
Copy link

@nyurik looks like your PR effectively got in (although indirectly) 🙂

@domoritz
Copy link
Member

Yep. Thank you @nyurik!

@nyurik nyurik deleted the es5 branch April 17, 2020 19:24
@nyurik
Copy link
Member Author

nyurik commented Apr 17, 2020

it's ok, only 4.5 months :)
Thanks for merging!

@domoritz
Copy link
Member

🦆 😇

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.

3 participants