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

Rewrote js/mage/translate.js without prototypejs #3662

Merged
merged 6 commits into from
Feb 11, 2024
Merged

Rewrote js/mage/translate.js without prototypejs #3662

merged 6 commits into from
Feb 11, 2024

Conversation

fballiano
Copy link
Contributor

needs extensive testing using language pack and checking that the frontend works as expected

@github-actions github-actions bot added the JavaScript Relates to js/* label Nov 18, 2023
@pquerner
Copy link
Contributor

What about ES5?

@kiatng
Copy link
Contributor

kiatng commented Nov 19, 2023

Yes! #3645 has started.

@luigifab
Copy link
Contributor

luigifab commented Nov 23, 2023

With ES6 syntax, we will instant crash all old browsers that does not understand the syntax.
With 1% of the file in ES6, even if 99% of the file is in ES5, the whole file is crashed.

https://caniuse.com/es6

@fballiano
Copy link
Contributor Author

sure but es6 support is 98.31%... should we not use it because of opera mini and IE11? then let's move everything to the next branch

@kiatng
Copy link
Contributor

kiatng commented Nov 24, 2023

sure but es6 support is 98.31%... should we not use it because of opera mini and IE11? then let's move everything to the next branch

next branch won't change anything on IE11. Opera Mini is only for android phones, the users can readily switch to the default android/chrome browser. So, it doesn't make sense to put this in next.

@pquerner
Copy link
Contributor

I think some other ticked mentioned transpiling.
In the core we can write in TypeScript and transpile it to ES5 or/and to ES6.

Or both and a feature flag decides whether or not to load the ES6 file.

I know that in our frontend no ES6 is allowed (sadly), since we have (still) lots of hits on older devices. And we don't want to break them (just yet?).

@kiatng
Copy link
Contributor

kiatng commented Nov 24, 2023

On @pquerner note, I vote for ES6 in next branch.

@fballiano fballiano changed the base branch from main to next November 24, 2023 09:48
@fballiano
Copy link
Contributor Author

rebased on next

@pquerner
Copy link
Contributor

By all means, I don't want to keep OM "behind" with JavaScript usage. One can always just fork the release and manually patch things, or revert commits. Just wanted to give my 2 cts. :)

@elidrissidev
Copy link
Member

elidrissidev commented Nov 24, 2023

Pretty sure I saw ES6 syntax in a merged PR before, so it might be too late to discuss that 😅 I say just keep using ES6 it's almost 2024!

Edit:

let searchInput = document.getElementById('search');


let upLevels = this._config.levels_up;

@kiatng
Copy link
Contributor

kiatng commented Nov 26, 2023

So, now what? Do we convert ES6 to ES5 in v20? Or do we carry on with ES6 in v20?

kiatng
kiatng previously approved these changes Jan 24, 2024
Copy link
Contributor

@kiatng kiatng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested in backend browser console:

> Translator.translate('Please wait, loading...')
< 'Please wait, loading...'
> Translator.add('abc', 'xyz')
< undefined
> Translator.translate('abc')
< 'xyz'
> Translator.add({'foo': 'bar'})
< undefined
> Translator.translate('foo')
< 'bar'

js/mage/translate.js Outdated Show resolved Hide resolved
@fballiano fballiano merged commit c105b6e into OpenMage:next Feb 11, 2024
1 check passed
@fballiano fballiano deleted the newtranslate branch February 11, 2024 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScript Relates to js/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants