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

fix(project): build issues on windows #4823

Merged
merged 21 commits into from
Jan 10, 2020
Merged

fix(project): build issues on windows #4823

merged 21 commits into from
Jan 10, 2020

Conversation

sichangi
Copy link
Contributor

@sichangi sichangi commented Dec 6, 2019

Closes #4809

Fixes build errors on running the build script on the windows operating system

Changelog

New

  • The react & vanilla icon builder checks the current operating system and handles path related operations accordingly
  • The same applies to the bundlers inline command
  • As well as the elements package build task
  • Clearer logging during builds

Changed

  • The react & vanilla icon builder script now generates icon variable names that start with a number, which throws an error while bundling, with an underscore on windows but retains initial behavior on other operating systems.

Testing / Reviewing

  • Should work as expected on any other previously functional operating system
  • Should generate similarly functioning outputs for react & vanilla icons

@sichangi sichangi requested a review from a team as a code owner December 6, 2019 08:54
@ghost ghost requested review from asudoh and dakahn December 6, 2019 08:54
@sichangi
Copy link
Contributor Author

sichangi commented Dec 6, 2019

Something that I may need an opinion on is the the fix I made where the icon bundlers throw on parsing variables that start with a number e.g 32Q....
I figured that the generated react icon files were exporting the icons by default ( export default ... ), I could add an underscore to the beginning of the variable name.
Am not so sure if this is fine for the vanilla output, hence the need for a second opinion.

@netlify
Copy link

netlify bot commented Dec 6, 2019 •
edited
Loading

Deploy preview for the-carbon-components ready!

Built with commit f8c5c99

https://deploy-preview-4823--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Dec 6, 2019 •
edited
Loading

Deploy preview for carbon-components-react ready!

Built with commit f8c5c99

https://deploy-preview-4823--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Dec 6, 2019 •
edited
Loading

Deploy preview for carbon-elements ready!

Built with commit f8c5c99

https://deploy-preview-4823--carbon-elements.netlify.com

joshblack
joshblack suggested changes Dec 6, 2019
View reviewed changes
packages/elements/tasks/build.js Outdated Show resolved Hide resolved
packages/icon-build-helpers/src/builders/react/builder.js Outdated Show resolved Hide resolved
packages/icon-build-helpers/src/builders/react/builder.js Outdated Show resolved Hide resolved
packages/bundler/src/commands/inline.js Outdated Show resolved Hide resolved
packages/bundler/src/commands/inline.js Outdated Show resolved Hide resolved
@github-actions github-actions bot added package: elements @carbon/elements package: icons-vue @carbon/icons-vue labels Jan 6, 2020
@joshblack
Copy link
Contributor

Bump @dakahn if you have a sec to help with reviewing this!

dakahn
dakahn approved these changes Jan 6, 2020
View reviewed changes
Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

seems good to me from my end 👍

joshblack
joshblack reviewed Jan 7, 2020
View reviewed changes
Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Thanks so much for sticking with this! Really appreciate it 🙏 Just had a couple of questions to see if we can reduce the number of times we rely on a windows-specific check 👀

packages/bundler/src/commands/inline.js Show resolved Hide resolved
packages/bundler/src/commands/inline.js Outdated Show resolved Hide resolved
@joshblack joshblack added status: ready to merge 🎉 and removed package: elements @carbon/elements package: icons-vue @carbon/icons-vue labels Jan 10, 2020
@joshblack joshblack merged commit 472bb65 into carbon-design-system:master Jan 10, 2020
@sichangi sichangi mentioned this pull request Jan 11, 2020
Closed
joshblack added a commit to joshblack/carbon that referenced this pull request Jan 13, 2020
@sichangi @mecolela @joshblack
chore(project): fix build issues on windows (carbon-design-system#4823)
9f02b35 
* fix(bundler): inline command path resolution in windows

* fix(elements): build tasks path resolution in windows

* fix(icon-build-helpers): react & vanilla builders path resolution in windows

* fix(icons-vue): path resolution in windows & improve logging

* fix(bundler): inconsistent sass module finder & code styling issues

* chore: use native path separator

* fix: moduleName prefixing logic

* chore: improve root directory check logic

Co-authored-by: mecolela <50064515+mecolela@users.noreply.github.com>
Co-authored-by: Josh Black <josh@josh.black>
@joshblack joshblack mentioned this pull request Jan 13, 2020
Closed
joshblack added a commit to joshblack/carbon that referenced this pull request Jan 14, 2020
@sichangi @mecolela @joshblack
chore(project): fix build issues on windows (carbon-design-system#4823)
a99142e 
* fix(bundler): inline command path resolution in windows

* fix(elements): build tasks path resolution in windows

* fix(icon-build-helpers): react & vanilla builders path resolution in windows

* fix(icons-vue): path resolution in windows & improve logging

* fix(bundler): inconsistent sass module finder & code styling issues

* chore: use native path separator

* fix: moduleName prefixing logic

* chore: improve root directory check logic

Co-authored-by: mecolela <50064515+mecolela@users.noreply.github.com>
Co-authored-by: Josh Black <josh@josh.black>
@joshblack joshblack mentioned this pull request Jan 14, 2020
Closed
@sichangi sichangi deleted the windows-support branch January 17, 2020 11:56
@joshblack joshblack mentioned this pull request Jan 31, 2020
Closed
@joshblack joshblack mentioned this pull request Feb 25, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dakahn dakahn dakahn approved these changes

@joshblack joshblack joshblack approved these changes

@asudoh asudoh Awaiting requested review from asudoh

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Windows Support
4 participants
@sichangi @joshblack @dakahn @mecolela