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

Remove babel requirement for server #1178

Merged
merged 4 commits into from
Nov 18, 2019

Conversation

ylecuyer
Copy link
Contributor

Remove babel requirement for server files

@jung-kim jung-kim mentioned this pull request Jan 21, 2019
@jung-kim
Copy link
Collaborator

I'm okay with removing babel but this means ungit will not work on older node or older browser as some of the client code within /components is in es6 now.

It would be nice to make a note of this in readme.

@ylecuyer
Copy link
Contributor Author

I may be wrong but the components assets are already served as es6. If I look at http://localhost:8448/plugins/app/app.bundle.js for example, I can see a fat arrow here :

    this.repoList.subscribe((newValue) => { storage.setItem('repositories', JSON.stringify(newValue)); });

And the grunt file calls browserify-components directly inside ./components which are not fed into babel (only source)

@campersau
Copy link
Collaborator

campersau commented Nov 12, 2019

It looks like @ylecuyer is right, the components were never fed into babel.
And since all tests passed on node 8 I think it is good.

I know its been a long time but do you mind rebasing this PR or creating a new one?
Otherwise I can do that for you.

@Hirse
Copy link
Contributor

Hirse commented Nov 12, 2019

Proposal:
Keep babel, but use it with preset-env for both server and components with different targets (and possibly "compile" shared files twice). This might end up being a no-op for Node, but that way we can use latest JS features and don't have to worry about support.

Side note, what are the supported browsers? I am getting syntax errors in IE11 from the fat arrows.

@campersau
Copy link
Collaborator

Its been over a year since #1137 got merged and we never got any issues regarding that.
So I would vote for just supporting evergreen browsers and make the build process a bit simpler :)

@campersau
Copy link
Collaborator

Thanks @ylecuyer !


If we still want to support older browsers we could use https://github.com/babel/babelify for example.

It is a bit difficult because octicons switched to a newer syntax and browserify ignores npm_modules by default and we use Array.prototype.includes, String.prototype.includes for which we would need a polyfill.

@campersau campersau merged commit f147963 into FredrikNoren:master Nov 18, 2019
@ylecuyer ylecuyer deleted the remove-babel-server branch January 7, 2020 21:21
@campersau campersau mentioned this pull request Apr 26, 2020
@campersau campersau mentioned this pull request May 14, 2020
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.

4 participants