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

Build fixes #153

Closed
wants to merge 7 commits into from
Closed

Build fixes #153

wants to merge 7 commits into from

Conversation

fkleuver
Copy link
Member

@fkleuver fkleuver commented Mar 5, 2018

This PR should completely fix the build and possibly some other problems which existed before, such as ux-components not fully working.

Steps (i used) to verify:

  • git clone https://github.com/fkleuver/ux.git fkleuver-ux
  • cd fkleuver-ux
  • git checkout build-fixes
  • npm install
  • .\node_modules\.bin\lerna bootstrap (I had to run this 3 times in a row before everything was done, this is due to a Windows-specific issue with lerna)
  • .\node_modules\.bin\lerna run build

And everything looks all green and happy again

Now, I'm a tad hesitant about the custom_typings approach I took but it seems like the least hacky solution. Those missing types simply don't exist in aurelia-binding.d.ts all the way up to 1.6.0, only just recently @bigopon added them to the master branch there. I doubt aurelia-ux will be updating the dependencies that high up anytime soon.

tsc also kept choking on the existing workaround ux-core/src/extension.ts - these .d.ts files I added are a replacement for that to fix the typing errors without affecting the build outputs. If you see a better way with less duplication and without overly hacky cross-references, I'd love to go with that instead. This is the best I could come up with for now.

@fkleuver
Copy link
Member Author

fkleuver commented Mar 6, 2018

I'll re-check this and make any necessary changes after #147 has been merged - if these changes are still necessary for a working build, that is

@bigopon
Copy link
Member

bigopon commented Mar 6, 2018

The extension.d.ts can be gone. I was using it with less strict tsconfig.json in my test. Should have chosen different approach.

@bigopon
Copy link
Member

bigopon commented Mar 6, 2018

From lerna doc,

lerna bootstrap --hoist

can give better performance, just in case it was a pain for you

@Alexander-Taran
Copy link

Alexander-Taran commented Mar 6, 2018

well since you mentioned --hoist .. there is explicit support for lerna in yarn workspaces..
might be of interest:
https://yarnpkg.com/blog/2017/08/02/introducing-workspaces/

as well as ability to specifically --noihoist something
https://yarnpkg.com/blog/2018/02/15/nohoist/

@fkleuver
Copy link
Member Author

fkleuver commented Mar 6, 2018

@bigopon Thanks for the tip, it did indeed took ages.. :)
@Alexander-Taran Does look interesting. I like yarn and use it in my own projects, but for aurelia I can imagine some drawbacks or at least significant work involved for switching. You ever brought this up in a separate issue?

@Alexander-Taran
Copy link

@bigopon I did not. I'm not that deep into aurelia development process. Can you share what drawbacks you forsee?

@fkleuver
Copy link
Member Author

fkleuver commented Mar 6, 2018

@Alexander-Taran Any minor change in the way dependencies are resolved, even if they are objectively improvements, could result in a different build output and potentially breaking changes for apps running in production. You never know for sure, it would need to be checked (hence significant work involved). Is probably something that needs to go in a next major release for the sake of safety.

@serifine
Copy link
Contributor

serifine commented Mar 6, 2018

As of right now, builds should all be working already.

@fkleuver
Copy link
Member Author

fkleuver commented Mar 6, 2018

Yep they do, nice.

Your call whether you still want to merge this PR. It still builds with the master merges. Would allow the current (AuBinding as any).ValueAttributeObserve workarounds to go back to normal usage which makes for cleaner typings.

I'd like to know whether you will or not though, so I know from which commit to proceed with the test setups if you want to go forward with those.

@serifine
Copy link
Contributor

serifine commented Mar 8, 2018

Been looking over this for a bit, going to not merge this one in. However I did merge in the testing changes.

@serifine serifine closed this Mar 8, 2018
@fkleuver
Copy link
Member Author

fkleuver commented Mar 8, 2018

@ZHollingshead There still seems to be the problem of components exporting a few things that aren't actually exported by those components. The build fails on this:
image
Is that a me-problem somehow, or shall I fix that in a separate PR?

Also still getting this:
node_modules/@types/lodash/index.d.ts(12651,53): error TS2344: Type 'T' does not satisfy the constraint 'object'.

Which is what skipLibCheck: true was meant to fix.

EDIT:
Never mind the lodash thing, that's just ux-select which is still based on the "old" template. I'll make a PR for that in a moment

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

Successfully merging this pull request may close these issues.

4 participants