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

[Bug]: Problem with old safari version #107

Closed
miltonlaufer opened this issue Mar 19, 2024 · 9 comments
Closed

[Bug]: Problem with old safari version #107

miltonlaufer opened this issue Mar 19, 2024 · 9 comments
Assignees
Labels
bug Something isn't working stale This has gone stale due to inactivity triage New issues get this label, remove it after triage

Comments

@miltonlaufer
Copy link

Provide environment information

Next.js 14.
No particular configuration.

Which project is this issue for?

@serwist/next

Link to reproduction - Issues with a link to complete (but minimal) reproduction code help us address them faster

Unfortunately it is a private repository.

To reproduce

Open a serwist website in Safari 14 or below and check the developer console.

Describe the bug

We've noticed that after deploying @Serwist/next we start getting frontend reports of errors like this

SyntaxError: Unexpected token '='. Expected an opening '(' before a method's parameter list.

After checking it, we realized it was only happening with MacOS X 10.15.6 and browser Apple Mail 605.1.15.

The error appeared in main.js and it was on the Serwist part.

The website doesn't load because of this.

After doing some research in the code, the problem comes from parts of the code like

class SerwistEvent {
    type;
    target;
    sw;
    originalEvent;
    isExternal;
    ...
}

or

    _eventListenerRegistry = new Map();
    ...
}

We've been able to fix it using next-transpile-modules, something like this

const withTM = require('next-transpile-modules')(['@serwist/window']);

/** @type {import('next').NextConfig} */
module.exports = withTM(withSerwist({

Of course, it is nice that it works with this workaround, but wanted to share the problem in case you weren't aware of it.

Expected behavior

To be able to load the website.

Screenshots (if relevant)

No response

Additional information (if relevant)

No response

@miltonlaufer miltonlaufer added bug Something isn't working triage New issues get this label, remove it after triage labels Mar 19, 2024
@DuCanhGH
Copy link
Member

DuCanhGH commented Mar 19, 2024

  1. It has been deprecated
    image

  2. That's not how you use it
    image

  3. Did you properly configure browserslist? If so, I recommend opening an issue in Next.js or SWC's repo.

  4. browser Apple Mail 605.1.15

    How in the flying hell is the Mail app supposed to run a service worker 💀 Ah, that seems to be related to @serwist/window.

@miltonlaufer
Copy link
Author

Thanks for your answer

  1. It has been deprecated
    This is a good information. What actually happened is that all its features have been integrated into Next since 13

image

Which is great, one library less.

  1. That's not how you use it

Not sure about this. It says it there
image

Also, it did fix my problem with @serwis not getting properly transpiled according to my browserslist
I used now this new Next.js built in feature for it. Otherwise @serwist doesn't work (neither the website) on Safari <=13.

  1. Did you properly configure browserslist? If so, I recommend opening an issue in Next.js or SWC's repo.

I did, but it seems that the transpilation of the node modules should happen in the libraries themselves.

  1. browser Apple Mail 605.1.15

    How in the flying hell is the Mail app supposed to run a service worker 💀 Ah, that seems to be related to @serwist/window.

Unfortunately that's how Sentry identifies Safari: Apple Mail XXX... Not ideal, I know

image

@DuCanhGH
Copy link
Member

DuCanhGH commented Mar 21, 2024

@miltonlaufer

Not sure about this. It says it there

Again, the use case of the package is for transpiling local modules, which are not transpiled by Next.js by default (most likely?). It is most likely intended for making such modules work on the server, since Node.js can't import i.e. TypeScript code, rather than for magically making your code compatible with older browsers. That is the job of webpack and SWC. Unless I'm missing out on some Next.js dark magic, it should transpile any library code for browsers coming its way.

I did, but it seems that the transpilation of the node modules should happen in the libraries themselves.

Not sure what you are talking about. This has always been the job of the transpiler and bundler you choose. You should consult them should you find an issue with how the code is generated.

@miltonlaufer
Copy link
Author

I might be wrong, for sure.

The thing is that it says there that it is for compiling node modules when using Next.JS. Of course Next does this taking into account your setting for the code of the application one is building, but I'm not sure about node_modules. If Next already did that, I'm not sure why this library exists (and why Next decided to added it to its core settings), but as I say I'd need to dig deeper into it.

I guess the main point is that @serwist wasn't working on old browsers until I added it to next.config.js

/** @type {import('next').NextConfig} */
module.exports = withSerwist({
	eslint: {
		ignoreDuringBuilds: true,
	},
	transpilePackages: ['@serwist/window', '@serwist/core'], // I needed to add this two
	swcMinify: true,

@DuCanhGH
Copy link
Member

DuCanhGH commented Mar 22, 2024

Of course Next does this taking into account your setting for the code of the application one is building, but I'm not sure about node_modules. If Next already did that, I'm not sure why this library exists (and why Next decided to added it to its core settings), but as I say I'd need to dig deeper into it.

It should take care of that. Otherwise, you may as well throw browserslist out, for it wouldn't be even remotely useful if I just straight up go and build the library to specifically target ESNext. Then again, I'm not that sure how sane Next.js is. For now, if the workaround works, so be it. But I'd like to reiterate my point: supporting older browsers is not the responsibility of library maintainers, for they haven't a slight context regarding how compatible the library consumers want their applications to be.

@miltonlaufer
Copy link
Author

miltonlaufer commented Mar 22, 2024

Man, knowing you put this project together, I'm pretty sure you are right.

But, come on, I'm just trying to help. I've been around for quite a bit and I am SURE if I have these problems (or the ones I posted on another issue) other people will too, so take it easy. We are just trying to help. Certain stuff in your previous responses were like "ok.... seems I cannot even point out a problem here..."

Thanks again for your effort and your responses. Honestly, just trying to to constructive stuff here.

@DuCanhGH
Copy link
Member

DuCanhGH commented Mar 22, 2024

@miltonlaufer

But, come on, I'm just trying to help. I've been around for quite a bit and I am SURE if I have these problems (or the ones I posted on another issue) other people will too, so take it easy. We are just trying to help. Certain stuff in your previous responses were like "ok.... seems I cannot even point out a problem here..."

Thanks again for your effort and your responses. Honestly, just trying to to constructive stuff here.

Sorry if I've been a bit too hostile previously, but the commuting was really bad today...

I do see the problem you are having right now, but I still think that it is on Next.js/SWC rather than Serwist.

I believe that Next.js does transpile the code, but seeing that your using transpilePackages works, I can't really be sure anymore. I really do hope that it is doing the right thing here...

I'm still inclined to believe that transpilePackages is meant for transpiling and bundling server code, just saying.

Copy link
Contributor

This issue is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale This has gone stale due to inactivity label Apr 22, 2024
Copy link
Contributor

github-actions bot commented May 6, 2024

This issue was closed because it has been inactive for 14 days since being marked as stale.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stale This has gone stale due to inactivity triage New issues get this label, remove it after triage
Projects
None yet
Development

No branches or pull requests

2 participants