-
Notifications
You must be signed in to change notification settings - Fork 79
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
Rollup for JS, Babel, Typescript and React with code formatting apis #69
base: master
Are you sure you want to change the base?
Conversation
Wow, impressive work @theajr ! I have trouble running the projects. React+typescripts gives the following error when I run
React+babel gives me the following error when running
|
@jakoblind let me check once. |
I have never seen that issue. Did it work before and suddenly stopped working? Which node version are you using? |
gatsby-node.js
Outdated
}; | ||
|
||
exports.onCreateWebpackConfig = ({ actions }) => { | ||
actions.setWebpackConfig({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment describing why we do this would be nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, as I mentioned, I was facing some error while using prettier api to format the code snippet. So this piece of code should be added here so that it works. I will keep all these comments and update the PR once my local systems issue is resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I see! a comment about this in the code will be very valuable once you get your systems up and running again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! My system issue is happening only with last commit. If I reset hard to previous commit and start the server, it is working fine. I tested the same branch with latest commit in my friend's mac, it is working fine! this is weird! I am setting up a ubuntu virutal machine to see if it helps.
@@ -259,6 +263,7 @@ export default class Features extends React.Component { | |||
onMouseLeave={onMouseLeave} | |||
selectedBuildTool={selectedBuildTool} | |||
key={group} | |||
expandByDefault={_.size(groupedFeatures) === 1} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong, but now that we have React, we don't need passing in expandByDefault
and the implementation in FeatureGruop
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can remove it. But having it won't hurt, in case we are going to add another tab in future this logic may be useful ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a valid point, however, in my experience all line of code adds up to creating complexity so I'm very careful not to have more code than necessary.
If you create a new commit reverting these lines, we'll have the code in git history and we can easily get them back if needed.
src/templates/rollup/index.js
Outdated
export const getIndex = configItems => { | ||
const isBabel = _.includes(configItems, 'Babel'); | ||
|
||
return getFormattedCode( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this call, in theory, be slow? If it makes external API calls or something? If it is then we need to consider doing caching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's just calling a js function which is inside utils folder. So it won't cause any delay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These apis are removed in latest commit, somehow the this commit is making some issue in my system. So I corrected the indentation manually. I will try to analyse more once this PR is merged.
It used to work my mac got crashed this morning, so restarted. From then, I am unable to start gatsby server. I was using node 10.x LTS version, I tried upgrading to 12.x LTS. still not working. It may take some time. |
ok maybe your error comes from the crash. It works fine for me on 10.x LTS. good luck fixing the mac, hope it isn't too difficult to fix |
86d3d93
to
8158bf1
Compare
@jakoblind I will let you once this PR is totally ready. I am working on it now. |
89ac3bb
to
9a657aa
Compare
@jakoblind It looks good now. I tested all rollup features from deployed netlify. |
I can reproduce the error you showed me yesterday on my mac. On my linux machine it works fine. Netlify also runs linux I think, that's why it also works there probably. I will help debugging this mac issue. |
@jakoblind Thanks. I tried the same in my friend's Mac, it was not throwing the error. I created a new Ubuntu virtual machine and there also I could see the issue. |
I pushed a fix to master. Please checkout and test. You might have to remove the This was the issue: There was a mismatch between version of NPM dependency
After some research I realized that the If everything works, you can merge in master to this branch |
…toconf into feature/rollup
…nt base in package json
@theajr I tested running React+babel and still get the same error as before, did you get a chance to look into why this error happens? Let me know if you need help debugging
|
I experimented a bit and I got it to work. Moving up
|
@jakoblind Thanks, I don't know why it is working at my end. Still I will add your changes and merge it. |
} | ||
plugins.push(`resolve({ | ||
extensions:${JSON.stringify(resolveExtensions)} | ||
}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a missing )
for resolve
default file should be |
@jakoblind Fixed both now. |
Now React+Babel works for me! Awesome! But I still cannot get React+typescript to work. Could you see if you can reproduce this error? running
|
@jakoblind Sure, I will look into it. |
I managed to reproduce on two machines now. But have not yet figured out how to fix it. If this takes to long, we could ship this without typescript support for now just to get it out. |
@jakoblind I will sit tonight to fix this. I wasn't getting much time recently. |
@jakoblind I found the fix. I will be pushing the code tomorrow. |
awesome @theajr ! looking forward to read the code. Have a good weekend! |
@jakoblind I will let you know once PR is ready to review. Have a good weekend. |
@jakoblind Please check now. |
Hmm. I still get the same error:
does it work for you? |
It's weird. It's always working for me. I never faced this issue. I wonder what is the deal here 😇 |
that's weird. what version of npm and node are you using? |
@jakoblind I could reproduce the issue in a actual windows machine(Working fine in windows in VM somehow). If I change the content of Code causing issue:
After removing data types, it is working fine. Can you please check the same and provide your thoughts?
It should support data types as well but seems to be an issue or I am missing something. |
Yes when I do that it's working fine. But then no typescript-specific code is running. I guess the problem is that this file is not run through the typescript compiler for some reason. That's why it works to remove the types. I'm still confused here. I can reproduce the error on two of my machines. I got a friend to run it and he couldn't reproduce the error. Anyway, I'll ask around a bit |
Is this the only blocker here ? |
@jakoblind Looks like last merge break build. |
@gengjiawen yeah this PR is now a bit outdated. There have been some refactorings in master. I have limited time, and will not prioritize fix this currently. |
@jakoblind
Added support for Rollup
Added code formatting apis, which are being used in formatting generated code snippets. We should use the same in webpack and parcel code generation as well. That will in be another PR along with changing sample typescript code in webpack and parcel as mentioned in last comments.