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

source mapping url comments stop safari #158

Closed
zhiwww opened this issue Sep 6, 2015 · 12 comments
Closed

source mapping url comments stop safari #158

zhiwww opened this issue Sep 6, 2015 · 12 comments

Comments

@zhiwww
Copy link

zhiwww commented Sep 6, 2015

//# sourceMappingURL=react.js.map

comments in all files under folder 'lib' stops safari. a 404 error interrupts everything.

condition:
1/ using webpack
2/ safari 9.0 (11601.1.43)

@ericf
Copy link
Collaborator

ericf commented Sep 11, 2015

See: #162

@ericf ericf closed this as completed Sep 11, 2015
@sirrodgepodge
Copy link

Hey Eric,

Love what you've made here!

Still getting this error in Safari (all other browsers work perfectly) though with React-Intl 2.0.0-beta2 and React 0.14 :(

Using Webpack and webpack dev server + hot loading (not doing anything special for handling react-intl, not sure if I should be)

[Error] Failed to load resource: the server responded with a status of 404 (Not Found) (main.js.map, line 0)

(running in localhost, Safari 9.0.3, Macbook Pro, OSX El Capitan 10.11.3)

@sirrodgepodge
Copy link

Also getting:

ReferenceError: No locale data has been provided for this object yet.

Along with the other error (only in Safari), perhaps related to the "map" issue as I know for sure that the locale data has been added given that it works fine in other browsers

@sirrodgepodge
Copy link

This may be the cause actually if the fix you mentioned above was to comment out the sourcemaps link (looks like Safari still evaluates commented out sourcemap links):
thlorenz/convert-source-map#31

@sirrodgepodge
Copy link

Whew, k so the sourcemap 404 still happens but it's not happening in production so all good.

In case anyone stumbles across this the real issue here is that for the safari "intl" polyfill you actually need to load in individual languages. Here is a slimmed down version of my final code (note window.intl.polyFill boolean):

enabledLanguages.js:

export default [
    'en',
    'ar',
    'es',
    'fr',
    'fr-CA',
    'po',
    'pt-BR',
    'ru',
    'th',
    'tr',
    'zh'
];

en.js(example language JSON):

export default {
    locale:"en",
    messages: {
        //// example content
        articleCard: {
            survey: {
                button: `Take The Survey`
            },
            quiz: {
                button: `Take The Quiz`
            },
            event: {
                seriesCaption: 'Event Series'
            }
        },
        article: {
            poll: {
                submitButton: `Submit & See Results`
            },
            survey: {
                successHeadline: `Thank you for your feedback!`,
            }
        },
        comment: `user {name} {value, plural,
              =0 {does not have any comments}
              =1 {has # comment}
              other {has # comments}
            }`,
        HTMLComment: `user <b style='font-weight: bold'>{name} </b> {value, plural,
              =0 {does not have <i style='font-style: italic'>any</i> comments}
              =1 {has <i style='font-style: italic'>#</i> comment}
              other {has <i style='font-style: italic'>#</i> comments}
            }`,
        nestedDateComment: `user {name} {value, plural,
              =0 {does not have any comments}
              =1 {has # comment}
              other {has # comments}
            } as of {date}`,

        //// real content
        nav: {
            greeting: `Hello, {name}`,
            topStories: `Top Stories`,
            mySubscriptions: `My Subscriptions`,
            bookmarks: `Bookmarks`,
            settings: `Settings`,
            logout: `Logout`,
            search : {
                placeholder: `Search...`,
              goButton: `Go`
            }
        },
        subscriptions: {
            heading: `My Subscriptions`,
            topics: `Topics`,
            sectorRegionFunction: `Sector, Region, Function`,
            custom: `Custom Subscriptions`,
            manage: `Manage Subscriptions`
        }
    }
}

localizationHandler.js:

    import {addLocaleData} from 'react-intl';
    import enabledLanguages from './enabledLanguages';

    // use Intl polyfill if not supported (in Safari)
    if (!window.Intl) {
        window.Intl = require('intl');
        window.Intl.polyFill = true;
    }

    const localizations = {};
    enabledLanguages.forEach((val) => {
        localizations[val] = require(`../../localization/${val}`); // if language-specific JSONs get too big, stop requiring/bunding in all languages and switch to using local API calls
        localizations[val].messages = flattenMessages(localizations[val].messages);
        window.Intl.polyFill && require(`intl/locale-data/jsonp/${localizations[val].locale}`);
        addLocaleData(require(`react-intl/lib/locale-data/${localizations[val].locale}`));
    });

    // use this if we want to allow nested messages
    function flattenMessages(nestedMessages = {}, prefix = '') {
        return Object.keys(nestedMessages).reduce((messages, key) => {
            let value       = nestedMessages[key];
            let prefixedKey = prefix ? `${prefix}.${key}` : key;

            if (typeof value === 'string') messages[prefixedKey] = value;
            else objectAssign(messages, flattenMessages(value, prefixedKey));

            return messages;
        }, {});
    }

    export default localizations;

Hey Eric, if I made a PR that added 'intl' polyfill (for Safari) and added the corresponding languages' 'intl' polyfills as part of the "addLocaleData" function would you accept?

This took me a good while to get to the bottom of I can't imagine anyone making a serious enough app to care about localization that would not care about browser compatibility with Safari.

@ericf
Copy link
Collaborator

ericf commented Feb 9, 2016

The source map not loading is likely orthogonal and not causing the actual code int he app to fail. I'm not the owner of the Intl polyfill, but your of course free to open an PR to improve it.

@sirrodgepodge
Copy link

Hey Eric,

Thank you for your quick response! Source maps is indeed orthogonal.

What I am proposing is a PR to react-intl itself that would:

#1: Make IntlProvider automatically include the Safari-required 'Intl' polyfill (at least by default, perhaps a flag could be passed as a prop to exclude if some odd user doesn't care about Safari working)

#2: Include adding the locale-specific polyfills (the real doozy, kept thinking I was missing something in react-intl itself) as part of the "addLocaleData" function

This might take me a while since I'm relatively new to React and you're doing some pretty nifty stuff so I want to get confirmation from you first.

Best,
Roger

@ericf
Copy link
Collaborator

ericf commented Feb 10, 2016

Make IntlProvider automatically include the Safari-required 'Intl' polyfill (at least by default, perhaps a flag could be passed as a prop to exclude if some odd user doesn't care about Safari working)

This isn't the approach that should be taken, React Intl assumes the Intl built-in. Use http://cdn.polyfill.io/v2/docs/ for automatic Intl polyfilling for Safari.

Include adding the locale-specific polyfills (the real doozy, kept thinking I was missing something in react-intl itself) as part of the "addLocaleData" function

I don't understand this one.

@sirrodgepodge
Copy link

k, fair enough on point #1, though I think it should be made very obvious in the docs (as opposed to having to search through issues) that Safari requires the "intl" npm as a polyfill to work

explaining #2, the thing that confused me for a while is that once I had added the "intl" npm polyfill I actually still needed to add each specific language that I wanted to use from this polyfill as well as those same specific languages from react-intl

IMO a note based on #2 should be included in the docs too as I'm sure this has tripped up plenty of folks that are super-excited about how well react-intl works in every other browser.

I've made a PR here: #322

@Dindaleon
Copy link

I tried @sirrodgepodge solution but I am still getting the failed to load error: unsupported URL error.

@sirrodgepodge
Copy link

Hey Dindaleon, just saw this, please post code if you could, would like to help

My first guess is that one of your included languages is named inconsistently with how it is named in the Intl package, e.g. "rs" instead of "ru" for Russian or something like that.

I've also updated my snippets above to include a sample localization file as I realize it was incomplete without.

@Dindaleon
Copy link

@sirrodgepodge I got my repo here: https://github.com/Dindaleon/hapi-react-starter-kit if you want to take a look; however, I have not updated it with your suggestions. I did try a solution provided in issue #71 and seems to be working fine. There, they talk about this webpack plugin that is stripping out the source map comments (https://www.npmjs.com/package/strip-sourcemap-loader) I Enabled it for my production build.

I am still waiting for a react-intl v2 update, as Eric said it was going to be fixed for next update.

EDIT @ericf I just tried RC 1 and want to say awesome job in speeding it up! It does feel 3 times faster as you mentioned it. Keep up the good work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants