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.

Sign up for GitHub

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

Jump to bottom

Error after updated to v7.3.0 #221

Closed
colloquet opened this issue Oct 4, 2017 · 13 comments
Closed

Error after updated to v7.3.0 #221

colloquet opened this issue Oct 4, 2017 · 13 comments

Comments

@colloquet
Copy link

Hi,

I have updated react-waypoint to 7.3.0 today and I am getting this error:

screen shot 2017-10-04 at 11 51 59 am

Everything is working fine after I revert back to v7.2.0

Is this a bug or am I missing something?

Thanks!
@trotzig
Copy link
Collaborator

trotzig commented Oct 4, 2017

The mjs extension was added (and discussed) in #220. I don't have much context on that change, so I'm going to ask @lencioni for help here. Thanks for taking the time to report this.

@lencioni
Copy link
Collaborator

lencioni commented Oct 4, 2017 via email

@colloquet
Copy link
Author

Hi @lencioni ,

I have quickly setup a react app using create-react-app:

https://github.com/colloquet/waypoint-test

I have only added react-waypoint v7.3.0 and a Waypoint component to App.js and still getting the same error.

Colloque

@dominikbulaj
Copy link

I have exactly the same problem after upgrading to 7.3.0
When I force version 7.2.0 it works. Same issue as in @colloquet case.

@waterizzet
Copy link

Unfortunately I've run into the same problem. After updating it to 7.3 it started giving me a similar error:

invariant.js:42 Uncaught Error: Invalid tag: /static/media/index.73a4b08a.mjs
at invariant (invariant.js:42)
at validateDangerousTag (ReactDOMComponent.js:343)
at new ReactDOMComponent (ReactDOMComponent.js:370)
at Object.createInternalComponent (ReactHostComponent.js:39)
at instantiateReactComponent (instantiateReactComponent.js:77)
...

@lencioni lencioni mentioned this issue Oct 4, 2017
Closed
@lencioni
Copy link
Collaborator

lencioni commented Oct 4, 2017

Thanks for the repo, it was very helpful.

I'm not super familiar with create-react-app, but it seems to me that its webpack has been configured to recognize files with names that end in .mjs as a static asset, so instead of returning a reference to what is being exported, it is outputing the file in static/media and giving you a string with the path to this file.

I opened an issue on create-react-app here: facebook/create-react-app#3237

In the short term, you can either stick with 7.2.0 for now, or import waypoint like import Waypoint from 'react-waypoint/build/index.js';

Sorry for the trouble, I hope we can get this resolved soon!

@jnsdls
Copy link

jnsdls commented Oct 5, 2017

same issue here, glad you reported it upstream to CRA, will track both that and this and stick with 7.2.0 for now :)

trotzig added a commit that referenced this issue Oct 5, 2017
@trotzig
Make ES module opt-in
0e791c0 
After releasing 7.3.0, we've seen folks run into issues (#221) with the
new `module` field added to package.json. I've tried to read up on how
webpack uses this field, and it looks like it will pick it up by
default. This is tricky because most people will expect a CommonJS
export (i.e. `module.exports = ...`), but now they suddenly have to
import react-waypoints using

  const Waypoint = require('react-waypoint').default;

There's a lengthy issue over at webpack discussing this [1], with a few
proposed solutions at the bottom:

- offer a separate package like lodash does (lodash-es), or
- skip the module field and use a es-specific entry point, like import a from 'yourpkg/es'
- skip the module field, avoid es modules (my preferred solution for most of my modules)

I'm opting for the second point here. We can use the commonjs bundle as
the default, and then if people are adventurous they can point at the es
module directly:

  import Waypoint from 'react-waypoint/build/index.mjs';

We're not really going to the bottom of this issue with this commit. But
I believe it will solve headaches for a lot of applications. For
instance, I just found out that one developer in my team spent time
chasing down a bug where parts of the application wouldn't work.
Waypoints that were imported with `import Waypoint from
'react-waypoint'` were working, but not the ones being imported via
`const Waypoint = require('react-waypoint')` (we have some legacy).

Fixes #221

[1] webpack/webpack#4742
@trotzig trotzig mentioned this issue Oct 5, 2017
Merged
@trotzig
Copy link
Collaborator

trotzig commented Oct 5, 2017

A fix for this was released in 7.3.1. Thanks for the quick reports everyone!

@colloquet
Copy link
Author

Thanks! Appreciate your help!

@benbrandt
Copy link

benbrandt commented Oct 16, 2017 •
edited
Loading

What is the motivation for going with .mjs? Most libraries that have /es/ imports still use .js. It just requires a lot of changes in the build process (webpack/eslint/jest/flow) to support an additional file extension, that from my understanding, is a new extension for node?
Just curious to get some insight on the decision. Thanks!

@jamesplease
Copy link
Collaborator

jamesplease commented Oct 16, 2017 •
edited
Loading

that from my understanding, is a new extension for node?

This is exactly right, and is the primary reason for the decision. Although it's still early on, there's a good chance that .mjs will one day be the extension that is typically associated with the usage of ES2015 modules. This library has just adopted the pattern a bit early.

It just requires a lot of changes in the build process (webpack/eslint/jest/flow) to support an additional file extension

I brought up these concerns when we merged in the PR that added this extension. The team decided in that PR that it's worth trying it out. My favorite part of the folks who work on Waypoint is that they're really reasonable. If the extension ends up causing too many issues for users, then I'm sure the team would consider switching back to .js, even if only for a period of time. For now, though, 7.3.1 seems stable. Have you run into specific issues with other tooling on the latest release?

@benbrandt
Copy link

I tried updating all of the places I could think of with the new extension and kept getting errors, so I gave up and stuck with cjs for now. I'll keep watching the repo to see what happens here and try again later

@jamesplease
Copy link
Collaborator

I tried updating all of the places I could think of with the new extension and kept getting errors, so I gave up and stuck with cjs for now.

Ah, alright. I'm sorry you're running into those issues, @benbrandt . If you post more details about what sorts of errors you were seeing with the .mjs extension, someone may be able to help out. Also, I'd be curious to know what happens if you manually go into node_modules and change both the package.json and source files to have a .js extension. There's a chance that your tooling is blowing up due to the ES modules syntax, rather than the file extension.

@trotzig trotzig mentioned this issue Dec 22, 2017
Merged
@realityking realityking mentioned this issue Jan 13, 2018
Closed
@realityking realityking mentioned this issue Nov 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@lencioni @trotzig @dominikbulaj @benbrandt @jamesplease @colloquet @jnsdls @waterizzet